|
|
Created:
Dec. 16, 2014, 11:11 p.m. by Wladimir Palant Modified:
Dec. 17, 2014, 4:49 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIn addition to the required change which is pretty trivial, this patch fixes two more issues: the hardcoded mapping is removed (this one wasn`t kept in sync and got outdated) and locales without any Chrome strings are no longer imported (makes little sense, affects only Marathi locale).
Patch Set 1 #
Total comments: 6
Patch Set 2 : Simplified mapping calculation #
Total comments: 2
Patch Set 3 : Removed code duplication #MessagesTotal messages: 10
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): Maybe I overlook something here? But it seems that this code can be simplified by generating the mapping the other way around: locale_mapping = {l: l for l in localeTools.chromeLocales} for chrome_locale, crowdin_locale in localeTools.langMappingChrome.iteritems(): locale_mapping[crowdin_locale] = chrome_locale for gecko_locale, crowdin_locale in localeTools.langMappingGecko.iteritems(): chrome_locale = locale_mapping.get(crowdin_locale) if chrome_locale is not None: locale_mapping[gecko_locale] = chrome_locale
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): On 2014/12/17 09:04:06, Sebastian Noack wrote: > Maybe I overlook something here? But it seems that this code can be simplified > by generating the mapping the other way around: This will produce lots of duplicates - we need each Chrome locale exactly once as value. Frankly, reversing the mapping isn't really necessary and we could keep keys as keys - I only did it to keep the original structure.
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): On 2014/12/17 10:09:54, Wladimir Palant wrote: > On 2014/12/17 09:04:06, Sebastian Noack wrote: > > Maybe I overlook something here? But it seems that this code can be simplified > > by generating the mapping the other way around: > > This will produce lots of duplicates - we need each Chrome locale exactly once > as value. Frankly, reversing the mapping isn't really necessary and we could > keep keys as keys - I only did it to keep the original structure. Well, I were hoping to get rid of get_key() by doing proper dict-lookups instead.
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): On 2014/12/17 10:18:36, Sebastian Noack wrote: > Well, I were hoping to get rid of get_key() by doing proper dict-lookups > instead. I think we can only do this by reversing the mapping in langMappingGecko - we can have a temporary dict instead of get_key.
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): On 2014/12/17 10:25:49, Wladimir Palant wrote: > On 2014/12/17 10:18:36, Sebastian Noack wrote: > > Well, I were hoping to get rid of get_key() by doing proper dict-lookups > > instead. > > I think we can only do this by reversing the mapping in langMappingGecko - we > can have a temporary dict instead of get_key. Both sounds good to me.
http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5629499534213120/pack... packagerChrome.py:179: def get_key(dict, value): On 2014/12/17 10:31:49, Sebastian Noack wrote: > Both sounds good to me. Ok, the current approach removes unnecessary steps.
http://codereview.adblockplus.org/5307739131609088/diff/5717271485874176/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5717271485874176/pack... packagerChrome.py:176: locale_mapping[chrome_locale.replace('-', '_')] = crowdin_locale I'd rather not duplicate the logic replacing the dashes, but moving it into a function or lambda function. convert_language_codes = lambda code: code.replace('-', '_')
http://codereview.adblockplus.org/5307739131609088/diff/5717271485874176/pack... File packagerChrome.py (right): http://codereview.adblockplus.org/5307739131609088/diff/5717271485874176/pack... packagerChrome.py:176: locale_mapping[chrome_locale.replace('-', '_')] = crowdin_locale On 2014/12/17 14:11:46, Sebastian Noack wrote: > I'd rather not duplicate the logic replacing the dashes, but moving it into a > function or lambda function. > > convert_language_codes = lambda code: code.replace('-', '_') Done.
LGTM |