Issue 29336281: Issue 2109 - Allow for translation of app independent repositories (Closed)
DescriptionIssue 2109 - Allow for translation of app independent repositories
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed Sebastian's feedback and removed unrelated changes #Patch Set 3 : Addressed Wladimir's initial feedback #
Total comments: 4
Patch Set 4 : Addressed some further feedback from Wladimir #
Total comments: 4
Patch Set 5 : Removed includeIncomplete parameter and fixed typo #Patch Set 6 : Fixed typo spotted during testing #MessagesTotal messages: 16
Patch Set 1
https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:111: if type == 'chrome' or type == 'ISO-15897': Nit: type in {'chrome', 'ISO-15897'} But why do we need to support both in the first place? https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:279: locales = set(map(lambda locale: mapLocale(localeConfig['name_format'], Why not using a set expression? {mapLocale(localeConfig['name_format'], locale) for locale in localeConfig['locales']} https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:381: fileHandle = codecs.open(path, 'rb', encoding='utf-8') Using the "with" statement? https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:448: fileHandle = codecs.open(path, 'wb', encoding='utf-8') As discussed before, please use io.open instead codecs.open. The API is more safe/explicit when it comes to unicode handling. And codecs.open() is currently been discussed to be deprecated.
Not done with the review, only some preliminary comments so far. https://codereview.adblockplus.org/29336281/diff/29336282/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode253 build.py:253: if type == 'chrome' or type == 'opera': What about Safari? https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode274 build.py:274: os.path.join(fullBasePath, locale) Replacing '_' by '-' is only necessary for ISO-15897 locales. In other words, you shouldn't do this for "non-Gecko" but rather check localeConfig['name_format']. https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode315 build.py:315: localeConfig, key) With localeConfig being very central for each localeTools call I would expect this parameter to go first (it replaces the type parameter). https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:111: if type == 'chrome' or type == 'ISO-15897': This logic is wrong - you cannot decide on the mapping based on both name_format and target_platforms, you have to choose one. In fact, what we care about here is name_format - not our target platform but rather which format we chose for locale naming. Consequently, langMappingChrome should be renamed into langMappingISO and langMappingGecko into langMappingBCP. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:286: locales.add(mapLocale('chrome', locale)) mapLocale('ISO-15897', ...) please, that's the format we have here. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:291: locales.add(mapLocale('gecko', match.group(1))) mapLocale('BCP-47', ...) please, that's the format we get here. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:297: locales.add(mapLocale('gecko', match2.group(1))) mapLocale('BCP-47', ...) please, that's the format we get here.
Patch Set 2 : Addressed Sebastian's feedback and removed unrelated changes https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:111: if type == 'chrome' or type == 'ISO-15897': On 2016/02/12 16:33:58, Sebastian Noack wrote: > Nit: type in {'chrome', 'ISO-15897'} > > But why do we need to support both in the first place? Done. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:279: locales = set(map(lambda locale: mapLocale(localeConfig['name_format'], On 2016/02/12 16:33:58, Sebastian Noack wrote: > Why not using a set expression? > > {mapLocale(localeConfig['name_format'], locale) for locale in > localeConfig['locales']} Good point, Done. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:381: fileHandle = codecs.open(path, 'rb', encoding='utf-8') On 2016/02/12 16:33:58, Sebastian Noack wrote: > Using the "with" statement? I really want to avoid unrelated changes this time. Infact I realise to that end refactoring the conditional logic here (and below) was a mistake. I've put it back. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:448: fileHandle = codecs.open(path, 'wb', encoding='utf-8') On 2016/02/12 16:33:58, Sebastian Noack wrote: > As discussed before, please use io.open instead codecs.open. The API is more > safe/explicit when it comes to unicode handling. And codecs.open() is currently > been discussed to be deprecated. As above, want to avoid unrelated changes and this code shouldn't have been even modified in the first place.
(By the way, I tested these changes quite thoroughly originally and I will again once the code review is done. Until then I'll just be doing small tests while I address all the feedback.)
https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:381: fileHandle = codecs.open(path, 'rb', encoding='utf-8') On 2016/02/12 17:28:15, kzar wrote: > On 2016/02/12 16:33:58, Sebastian Noack wrote: > > Using the "with" statement? > > I really want to avoid unrelated changes this time. Infact I realise to that end > refactoring the conditional logic here (and below) was a mistake. I've put it > back. Well, apparently you change this code anyway. Not too important though. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:448: fileHandle = codecs.open(path, 'wb', encoding='utf-8') On 2016/02/12 17:28:15, kzar wrote: > On 2016/02/12 16:33:58, Sebastian Noack wrote: > > As discussed before, please use io.open instead codecs.open. The API is more > > safe/explicit when it comes to unicode handling. And codecs.open() is > currently > > been discussed to be deprecated. > > As above, want to avoid unrelated changes and this code shouldn't have been even > modified in the first place. Also here, as you change that code anyway, it's good time to tidy it up. But I guess it's not too important.
Patch Set 3 : Addressed Wladimir's initial feedback https://codereview.adblockplus.org/29336281/diff/29336282/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode253 build.py:253: if type == 'chrome' or type == 'opera': On 2016/02/12 17:13:04, Wladimir Palant wrote: > What about Safari? Safari isn't a supported type for any of the translation commands. https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode274 build.py:274: os.path.join(fullBasePath, locale) On 2016/02/12 17:13:04, Wladimir Palant wrote: > Replacing '_' by '-' is only necessary for ISO-15897 locales. In other words, > you shouldn't do this for "non-Gecko" but rather check > localeConfig['name_format']. Done. https://codereview.adblockplus.org/29336281/diff/29336282/build.py#newcode315 build.py:315: localeConfig, key) On 2016/02/12 17:13:04, Wladimir Palant wrote: > With localeConfig being very central for each localeTools call I would expect > this parameter to go first (it replaces the type parameter). Done. https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336282/localeTools.py#newc... localeTools.py:286: locales.add(mapLocale('chrome', locale)) On 2016/02/12 17:13:04, Wladimir Palant wrote: > mapLocale('ISO-15897', ...) please, that's the format we have here. Yep, already changed those in second patch set. Same below.
LGTM from my side. But It sounds like Wladimir isn't done yet?
https://codereview.adblockplus.org/29336281/diff/29336309/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336309/build.py#newcode251 build.py:251: } Gecko is still being treated as a special case here, for no good reason IMHO. I think that it should be something like this: if type == 'gecko': localeDir = packager.getLocalesDir(baseDir) ... elif type == 'chrome' or type == 'opera': localeDir = os.path.join(baseDir, '_locales') ... else: localeDir = os.path.join(baseDir, *metadata.get('locales', 'base_path').split('/')) ... localeConfig['base_path'] = localeDir locales = packager.getLocales(baseDir, includeIncomplete) if type == 'gecko' else os.listdir(localeDir) ... Note that localeDir is always absolute, not relative. Also, I think that explicitly splitting base_path setting from metadata by forward slash is a good idea - the paths specified there aren't supposed to be OS-dependent. I presume that os.path.join(baseDir, 'foo/bar') will still do the right thing on Windows but I'd rather not rely on this unnecessarily. Sebastian has the last word here of course but so far we've been using this approach consistently throughout buildtools. Note for future: we should seriously consider implementing the same kind of locale validation that packager.getLocales() performs for other platforms as well. https://codereview.adblockplus.org/29336281/diff/29336309/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336309/localeTools.py#newc... localeTools.py:404: if not re.match(r'^[\w\-]+$', dir) or dir == localeConfig['default_locale']: This check won't work correctly for Chrome, we will be comparing 'en-US' to 'en_US' here. We should compare to mapLocale(localeConfig['name_format'], localeConfig['default_locale']) instead.
Patch Set 4 : Addressed some further feedback from Wladimir https://codereview.adblockplus.org/29336281/diff/29336309/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336309/build.py#newcode251 build.py:251: } On 2016/02/12 19:05:43, Wladimir Palant wrote: > Gecko is still being treated as a special case here, for no good reason IMHO. I > think that it should be something like this: > > if type == 'gecko': > localeDir = packager.getLocalesDir(baseDir) > ... > elif type == 'chrome' or type == 'opera': > localeDir = os.path.join(baseDir, '_locales') > ... > else: > localeDir = os.path.join(baseDir, *metadata.get('locales', > 'base_path').split('/')) > ... > > localeConfig['base_path'] = localeDir > locales = packager.getLocales(baseDir, includeIncomplete) if type == 'gecko' > else os.listdir(localeDir) > ... > > Note that localeDir is always absolute, not relative. Also, I think that > explicitly splitting base_path setting from metadata by forward slash is a good > idea - the paths specified there aren't supposed to be OS-dependent. I presume > that os.path.join(baseDir, 'foo/bar') will still do the right thing on Windows > but I'd rather not rely on this unnecessarily. Sebastian has the last word here > of course but so far we've been using this approach consistently throughout > buildtools. > > Note for future: we should seriously consider implementing the same kind of > locale validation that packager.getLocales() performs for other platforms as > well. Done. https://codereview.adblockplus.org/29336281/diff/29336309/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336309/localeTools.py#newc... localeTools.py:404: if not re.match(r'^[\w\-]+$', dir) or dir == localeConfig['default_locale']: On 2016/02/12 19:05:44, Wladimir Palant wrote: > This check won't work correctly for Chrome, we will be comparing 'en-US' to > 'en_US' here. We should compare to mapLocale(localeConfig['name_format'], > localeConfig['default_locale']) instead. Good point but I think we also need to replace the underscores. Done.
https://codereview.adblockplus.org/29336281/diff/29336312/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336312/build.py#newcode272 build.py:272: for locale in os.listdir(localeDir)] This is actually fine for the use cases so far, but the includeIncomplete parameter is unused now - please remove. https://codereview.adblockplus.org/29336281/diff/29336312/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336312/localeTools.py#newc... localeTools.py:403: normalizedDefaultLocale = mapLocale('ISO-15897', normalizedDefaultLocale) It shouldn't be 'ISO-15897' but localeConfig['name_format'] here.
Patch Set 5 : Removed includeIncomplete parameter and fixed typo https://codereview.adblockplus.org/29336281/diff/29336312/build.py File build.py (right): https://codereview.adblockplus.org/29336281/diff/29336312/build.py#newcode272 build.py:272: for locale in os.listdir(localeDir)] On 2016/02/12 19:49:34, Wladimir Palant wrote: > This is actually fine for the use cases so far, but the includeIncomplete > parameter is unused now - please remove. Done but I omitted this parameter accidentally, are you sure that it was superfluous? https://codereview.adblockplus.org/29336281/diff/29336312/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29336281/diff/29336312/localeTools.py#newc... localeTools.py:403: normalizedDefaultLocale = mapLocale('ISO-15897', normalizedDefaultLocale) On 2016/02/12 19:49:34, Wladimir Palant wrote: > It shouldn't be 'ISO-15897' but localeConfig['name_format'] here. Whoops sorry, Done.
LGTM
LGTM
Patch Set 6 : Fixed typo spotted during testing
Message was sent while issue was closed.
Still LGTM 
|
