Re: [gentoo-dev] Re: RFC: New eclass: mozlinguas.eclass

[Nirbheek Chauhan]

On Fri, Feb 3, 2012 at 3:26 PM, Mike Frysinger <vapier@g.o> wrote:
> please post it inline to make review easier
>
>> # @MAINTAINER: mozilla@g.o
>> # @AUTHOR: Nirbheek Chauhan <nirbheek@g.o>
>
> goes on newline, not inlined
>

Fixed

>> # @DESCRIPTION: Array containing the list of language pack xpis available
>
> text starts on the next line, not the existing line
>

Fixed

>> # @ECLASS-VARIABLE: LANGS
>> # @ECLASS-VARIABLE: LANGPACK_PREFIX
>> # @ECLASS-VARIABLE: LANGPACK_SUFFIX
>
> these prob could use MOZ prefixes as well
>

Fixed

>> : ${LANGS:=""}
>
> you say it's an array but then you initialize it to a string ...
>

Meh, no real difference. :p

Changed anyway!

>> if ! [[ ${PV} =~ alpha|beta ]]; then
>>       for x in "${LANGS[@]}" ; do
>
> x is a global variable here ... one reason to write this as an internal func
> and then call it so you can use `local`
>

I just added an "unset x" at the end of the chunk, that should be
sufficient I think.

>>               if [[ ${x} = en ]] || [[ ${x} = en-US ]]; then
>
> should be == imo
>

Fixed

>>               SRC_URI="${SRC_URI}
>
> SRC_URI+="...
>

Fixed

>>               IUSE="${IUSE} linguas_${x/-/_}"
>
> IUSE+="...
>

Fixed

>> mozlinguas() {
>
> missing eclass documentation
>

Is it really needed for private functions? Nothing should ever call this.

>>       # Generate the list of language packs called "linguas"
>>       # This list is used to unpack and install the xpi language packs
>
> shouldn't this initialize linguas=() ?
>
> and shouldn't it name the return value mozlinguas ?
>

Fixed, and renamed the function to mozlinguas_export()

>>               # If this language is supported by ${P},
>>               elif has ${lingua} "${LANGS[@]//-/_}"; then
>>                       # Add the language to linguas, if it isn't already there
>>                       has ${lingua//_/-} "${linguas[@]}" || linguas+=(${lingua//_/-})
>>                       continue
>>               # For each short lingua that isn't in LANGS,
>>               # We used to add *all* long LANGS to the linguas list,
>>               # but we stopped doing that due to bug 325195.
>>               fi
>
> indentation on these comments seem to be off
>

No, that's on purpose. There used to be an `else` statement there.
That comment doesn't belong to the previous `elif` block. I've added
it outside a blank else block to clarify that.

>>               # FIXME: Add support for unpacking xpis to portage
>>               xpi_unpack "${MOZ_P}-${x}.xpi"
>
> or, add it to the new unpacker.eclass ;)
>
> also, seems to be missing `use linguas_${x} && xpi_unpack ...` ?  otherwise,
> you just unpack all linguas and not just the ones the user has requested ...
> same goes for the install ...
>

No, "${mozlinguas[@]}" is already the intersection of MOZ_LANGS and LINGUAS.

>>       if [[ "${linguas[*]}" != "" && "${linguas[*]}" != "en" ]]; then
>>               einfo "Selected language packs (first will be default): ${linguas[*]}"
>
> since linguas[*] will be big by default, i'd put the variable expansion into
> its own einfo

It's actually really small by default since it's the list of enabled langpacks.

Fixed version attached, thanks for the review!

-- 
~Nirbheek Chauhan

Gentoo GNOME+Mozilla Team