nirbheek at gentoo
Feb 3, 2012, 8:44 AM
Post #5 of 8
(436 views)
Permalink
|
|
Re: Re: RFC: New eclass: mozlinguas.eclass
[In reply to]
|
|
On Fri, Feb 3, 2012 at 3:26 PM, Mike Frysinger <vapier [at] gentoo> wrote:
> please post it inline to make review easier
>
>> # @MAINTAINER: mozilla [at] gentoo
>> # @AUTHOR: Nirbheek Chauhan <nirbheek [at] gentoo>
>
> 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
|
mozlinguas.eclass
mozlinguas.eclass.txt
