Issue 29561557: Issue 5763 - Target languages supported by Firefox (Closed)
|
Created:
Oct. 1, 2017, 5:12 p.m. by Sebastian Noack Modified:
Oct. 6, 2017, 5:21 p.m. Visibility:
Public. |
DescriptionIssue 5763 - Target languages supported by Firefox
Patch Set 1 #
Total comments: 10
Patch Set 2 : Adapted setuptrans + addressed nits #Patch Set 3 : Adressed Vasily's comments #Patch Set 4 : Removed unused target_platforms key #
Total comments: 2
Patch Set 5 : Removed redundant whitespace and fixed unrelated bug #
Total comments: 1
Patch Set 6 : Fixed import_locales() #
Total comments: 5
Patch Set 7 : Fixed undefined variable, put URLS in globals, removed redundand flake8 ignores #
Total comments: 4
MessagesTotal messages: 13
Hey Sebastian, please find the comments below. Best, Tristan https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py#newc... localeTools.py:64: 'es-419', iirc, removing the mapping for this will trigger a warning when someone would run "setuptrans" - do we ignore this? https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:299: def fixTranslationsForChrome(files): nit: when changing a function's name the new name should adhere to PEP-8 (lowercase, underscores). https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:318: # The Chrome Web Store requires messages used in manifest.json to be nit: this line is 1 character too long
Hi Sebastian, It looks pretty good to me other than a couple of rather philosophical comments (see below ;) Cheers, Vasily https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:315: if match: This loop with a regexp search and then `if match` creates an annoying additional level of iteration that doesn't really represent anything useful and just gets in the way. You could probably get rid of it this way: for path in list(files): match = ... if not match: continue ... although I admit that this inverted conditional is not quite satisfactory. What I would like to have is something like: for path, (isLatAm, isMexican, filename) in re.magic_filter(REGEX, files): ... but there doesn't seem to be any way to get to anything resembling this and still readable and sensible. Well, I guess I leave it up to you to decide if you like the inverted conditional more than the extraneous indent. https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:316: isLatAm, isMexican, filename = match.groups() While PEP8 is annoyingly non-specific with regards to the naming convention for local variables, I've always assumed it's supposed to be lowercase with underscores (to be consistent with functions[1] and global variables[2]). What do you think the convention should be (and maybe we need to add it to our style guide to augment this missing part of PEP8)? [1] https://www.python.org/dev/peps/pep-0008/#function-names [2] https://www.python.org/dev/peps/pep-0008/#global-variable-names
On 2017/10/02 21:13:32, Vasily Kuznetsov wrote: > This loop with a regexp search and then `if match` creates an annoying > additional level of iteration that doesn't really represent anything useful and I meant additional level of _indentation_.
https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py#newc... localeTools.py:64: 'es-419', On 2017/10/02 09:25:43, tlucas wrote: > iirc, removing the mapping for this will trigger a warning when someone would > run "setuptrans" - do we ignore this? The behavior of setuptrans is outdated. I initially planned to address this separately but here we go. 1. The hard-coded list of chrome locales is outdated. Unfortunately, there is no official source we can get these data from on-the-fly, but there is a quite reliable third-party source, i.e. Debian. ;) 2. The distinction between Gecko and Chrome locales is outdated. We no longer have Chrome-only or Gecko-only extensions, so all translation projects need to be translated into all languages supported by any of the two. 3. There have been some further inconsistencies where Firefox supports different dialects of a language, but we only provide one translation (e.g. Firefox has "bn-BD" and "bn-IN", but we only translate "bn"). I did changes to setupTranslations() to account for all of these, and I confirmed that the languages it would add to a project are tha same languages we currently have in our translation projects. It still gives one warning though, about "cak" (Kaqchikel), since Crowdin doesn't support this language at the moment, but Firefox does. However, note that this warning isn't new. https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:299: def fixTranslationsForChrome(files): On 2017/10/02 09:25:43, tlucas wrote: > nit: when changing a function's name the new name should adhere to PEP-8 > (lowercase, underscores). Done. https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:315: if match: On 2017/10/02 21:13:32, Vasily Kuznetsov wrote: > This loop with a regexp search and then `if match` creates an annoying > additional level of iteration that doesn't really represent anything useful and > just gets in the way. You could probably get rid of it this way: > > for path in list(files): > match = ... > if not match: > continue > ... > > although I admit that this inverted conditional is not quite satisfactory. What > I would like to have is something like: > > for path, (isLatAm, isMexican, filename) in re.magic_filter(REGEX, files): > ... > > but there doesn't seem to be any way to get to anything resembling this and > still readable and sensible. Well, I guess I leave it up to you to decide if you > like the inverted conditional more than the extraneous indent. The reason why I didn't bail out, but went for the extra level of indentation was that it looked weird to either have `... = match.groups()` surrounded by blank lines or have it squashed together with the block below. But otherwise, I would have gone for using continue as well. Anyway, here you go. https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:316: isLatAm, isMexican, filename = match.groups() On 2017/10/02 21:13:32, Vasily Kuznetsov wrote: > While PEP8 is annoyingly non-specific with regards to the naming convention for > local variables, I've always assumed it's supposed to be lowercase with > underscores (to be consistent with functions[1] and global variables[2]). What > do you think the convention should be (and maybe we need to add it to our style > guide to augment this missing part of PEP8)? > > [1] https://www.python.org/dev/peps/pep-0008/#function-names > [2] https://www.python.org/dev/peps/pep-0008/#global-variable-names Yeah, this should be lowercase with underscores. Done. https://codereview.adblockplus.org/29561557/diff/29561558/packagerChrome.py#n... packagerChrome.py:318: # The Chrome Web Store requires messages used in manifest.json to be On 2017/10/02 09:25:43, tlucas wrote: > nit: this line is 1 character too long Done.
Hey Sebastian, looks pretty good ( and actually takes a lot of work from 5751 ) - there's only 1 flake8 error, please see below. Best, Tristan https://codereview.adblockplus.org/29561557/diff/29563610/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29563610/localeTools.py#newc... localeTools.py:271: # Add languages with existing translations. flake8 fails on this line: W291 trailing whitespace
By the way, this just popped into my mind: Have you tried running "setuptrans", "translate", "uploadtrans" and "gettranslations" with this code for e.g. our abpchrome repo with our testproject setup as the target project?
On 2017/10/04 10:23:51, tlucas wrote: > By the way, this just popped into my mind: Have you tried running "setuptrans", > "translate", "uploadtrans" and "gettranslations" with this code for e.g. our > abpchrome repo with our testproject setup as the target project? Good idea. So far I only tested the setuptrans command. I did tested the all the other commands as well now. I found a somewhat unrelated bug, see the inline comment. https://codereview.adblockplus.org/29561557/diff/29563610/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29563610/localeTools.py#newc... localeTools.py:271: # Add languages with existing translations. On 2017/10/04 09:37:39, tlucas wrote: > flake8 fails on this line: W291 trailing whitespace Done. https://codereview.adblockplus.org/29561557/diff/29564752/build.py File build.py (right): https://codereview.adblockplus.org/29561557/diff/29564752/build.py#newcode343 build.py:343: if locale != localeConfig['default_locale'].replace('_', '-'): uploadtrans was failing because en-US doesn't exist as a target language. Note that before setuptrans also mistakenly added the source/default language as a target language, so if you did run setuptrans+translate+uploadtrans before, this error wouldn't occur. But the actual bug here isn't new, which is that "default_locale" uses either underscores or dashes based on the format used, while "locales" has been already normalized to use dashes. So the check here never worked for the ISO format.
LGTM
https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py File packagerChrome.py (left): https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py#o... packagerChrome.py:246: if not os.path.exists(sourceFile) or os.path.exists(incompleteMarker): I didn't care to keep supporting the .incomplete marker, as this feature is specific to legacy Gecko extensions. https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py#o... packagerChrome.py:249: data = json.loads(files[targetFile].decode('utf-8')) I only tested the translation commands after removing localetools.chromeLocales, but just realized that this variable is also used by the code here, when building the extension. Previously, we went to quite some length to compile a list of locales supported by Chrome and the respective language code used by the legacy Gecko extension. This was necessary as we were importing strings from the "adblockplus" repository. Also adblockplusui is using the same notation, but since the legacy Gecko extension is gone we can just rename the translation directories in adblockplusui, to match what we have in adblockpluschrome, now. Anyway, there no longer is a known list of languages supported by Chrome, and relying on such a list would no longer be accurate, as we now also need translations for all languages supported by Firefox. So instead I'm now importing every available translation, assuming the language code matches. https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py#n... packagerChrome.py:221: locale = sourceFile.split(os.path.sep)[-2].replace('-', '_') Replacing dashes with underscore here is only necessary to support the BCP-47 format. Pelase remove this replacement with your patch that removes support for the BCP-47 format.
Hi Sebastian, I wouldn't be able to confirm the validity of some of the logic changes, like in `import_locales`, but I trust you that they make sense -- you're the extension developer here. LGTM and I like all the code removal, always good to see that :D Just one inline comment below, but I leave it up to you whether you would like to act on it. Cheers, Vasily https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py#newc... localeTools.py:246: data = urllib2.urlopen('http://www.mozilla.org/en-US/firefox/all.html').read() What do you think about moving all these long URLs into constants at the beginning of the file? This way we'd get rid of the hardcoded strings and of the lines that are too long in this function at the same time.
https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py#newc... localeTools.py:246: data = urllib2.urlopen('http://www.mozilla.org/en-US/firefox/all.html').read() On 2017/10/05 20:26:03, Vasily Kuznetsov wrote: > What do you think about moving all these long URLs into constants at the > beginning of the file? This way we'd get rid of the hardcoded strings and of the > lines that are too long in this function at the same time. Done. https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py#n... packagerChrome.py:221: parts = sourceFile.split(os.path.sep) I just noticed that the variable "parts" is used below. I didn't notice before, because it's only used in the branch for legacy Gecko extensions below. @Tristan: Feel free to remove this variable again when removing support for legacy Gecko extensions. https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini#newcode12 tox.ini:12: packagerChrome.py : A101,A103,A104,A107,A108,A111,A112,A302,E501,E711,F841,N802,N803,N806 These ignores are no longer necessary. @Tristan: It might be worth to check again whether we can reduce this list even further after your changes.
Hey Sebastian, still LGTM. https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py#n... packagerChrome.py:221: parts = sourceFile.split(os.path.sep) On 2017/10/05 20:58:38, Sebastian Noack wrote: > I just noticed that the variable "parts" is used below. I didn't notice before, > because it's only used in the branch for legacy Gecko extensions below. > > @Tristan: Feel free to remove this variable again when removing support for > legacy Gecko extensions. Acknowledged. https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini#newcode12 tox.ini:12: packagerChrome.py : A101,A103,A104,A107,A108,A111,A112,A302,E501,E711,F841,N802,N803,N806 On 2017/10/05 20:58:39, Sebastian Noack wrote: > These ignores are no longer necessary. > > @Tristan: It might be worth to check again whether we can reduce this list even > further after your changes. Good idea, will do. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
