please post it inline to make review easier > # @MAINTAINER: mozi...@gentoo.org > # @AUTHOR: Nirbheek Chauhan <nirbh...@gentoo.org>
goes on newline, not inlined
> # @DESCRIPTION: Array containing the list of language pack xpis available
text starts on the next line, not the existing line
> # @ECLASS-VARIABLE: LANGS
> # @ECLASS-VARIABLE: LANGPACK_PREFIX
> # @ECLASS-VARIABLE: LANGPACK_SUFFIX
these prob could use MOZ prefixes as well
> : ${LANGS:=""}
you say it's an array but then you initialize it to a string ...
> 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`
> if [[ ${x} = en ]] || [[ ${x} = en-US ]]; then
should be == imo
> SRC_URI="${SRC_URI}
SRC_URI+="...
> IUSE="${IUSE} linguas_${x/-/_}"
IUSE+="...
> mozlinguas() {
missing eclass documentation
> # 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 ?
> # 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
> # 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 ...
> 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
-mike
signature.asc
Description: This is a digitally signed message part.
