Issue 29328085: Issue 3076 - [CMS] Map locale names to match Crowdin's expectations (Closed)
|
Created:
Sept. 16, 2015, 4:27 p.m. by kzar Modified:
Sept. 17, 2015, 3:08 p.m. Reviewers:
Sebastian Noack CC:
Wladimir Palant, saroyanm Visibility:
Public. |
DescriptionIssue 3076 - [CMS] Map locale names to match Crowdin's expectations
Patch Set 1 #
Total comments: 9
Patch Set 2 : Simplified local_locales assignment #Patch Set 3 : Simplify locale name conversion logic #
Total comments: 5
Patch Set 4 : Addressed nits #MessagesTotal messages: 10
Patch Set 1
https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:133: if "_" in locale: This can be simplified: if "_" in locale: crowdin_locale = locale.replace("_", "-") elif locale in supported_locales: crowdin_locale = locale else: crowdin_locale = "%s-%s" % (locale, locale.upper()) https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:270: with archive.open(member) as f, open(output_path, "wb") as f_target: Why not |archive.extract(member, output_path)|? https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:285: local_locales = {l for l in source.list_locales() if l != defaultlocale} Nit: set(source.list_locales()) - {defaultlocale}
Patch Set 2 : Simplified local_locales assignment https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:133: if "_" in locale: On 2015/09/16 19:37:37, Sebastian Noack wrote: > This can be simplified: > > if "_" in locale: > crowdin_locale = locale.replace("_", "-") > elif locale in supported_locales: > crowdin_locale = locale > else: > crowdin_locale = "%s-%s" % (locale, locale.upper()) Yea, I did it kinda like that initially but then it bugged me that if locale == crowdin_locale we're doing the "in supported_locales" check twice. I'm not sure which is better, maybe you're right. https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:270: with archive.open(member) as f, open(output_path, "wb") as f_target: On 2015/09/16 19:37:37, Sebastian Noack wrote: > Why not |archive.extract(member, output_path)|? I believe the path parameter to the extract method is just for the base path, the path of the file inside the archive and the file name is joined to that. So if we did what you're suggestion wouldn't the path end up being something like ".../locales/es/some_page.json/es-ES/some_page.json"? https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:285: local_locales = {l for l in source.list_locales() if l != defaultlocale} On 2015/09/16 19:37:37, Sebastian Noack wrote: > Nit: set(source.list_locales()) - {defaultlocale} Done.
https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:270: with archive.open(member) as f, open(output_path, "wb") as f_target: On 2015/09/16 20:02:41, kzar wrote: > On 2015/09/16 19:37:37, Sebastian Noack wrote: > > Why not |archive.extract(member, output_path)|? > > I believe the path parameter to the extract method is just for the base path, > the path of the file inside the archive and the file name is joined to that. So > if we did what you're suggestion wouldn't the path end up being something like > ".../locales/es/some_page.json/es-ES/some_page.json"? (I just tested that to make sure, unfortunately my fears where confirmed :/ )
https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:133: if "_" in locale: On 2015/09/16 20:02:41, kzar wrote: > On 2015/09/16 19:37:37, Sebastian Noack wrote: > > This can be simplified: > > > > if "_" in locale: > > crowdin_locale = locale.replace("_", "-") > > elif locale in supported_locales: > > crowdin_locale = locale > > else: > > crowdin_locale = "%s-%s" % (locale, locale.upper()) > > Yea, I did it kinda like that initially but then it bugged me that if locale == > crowdin_locale we're doing the "in supported_locales" check twice. I'm not sure > which is better, maybe you're right. Complexity-wise you have two |in supported_locales| checks either way. But with the current implementation you are duplicating |required_locales[locale] =| as well and have an unnecessary loop control instruction, making the code flow harder to follow. And performance-wise, set lookups have constant time. Not that performance matters here anyway.
Patch Set 3 : Simplify locale name conversion logic https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328086/cms/bin/translate.p... cms/bin/translate.py:133: if "_" in locale: On 2015/09/17 09:45:14, Sebastian Noack wrote: > On 2015/09/16 20:02:41, kzar wrote: > > On 2015/09/16 19:37:37, Sebastian Noack wrote: > > > This can be simplified: > > > > > > if "_" in locale: > > > crowdin_locale = locale.replace("_", "-") > > > elif locale in supported_locales: > > > crowdin_locale = locale > > > else: > > > crowdin_locale = "%s-%s" % (locale, locale.upper()) > > > > Yea, I did it kinda like that initially but then it bugged me that if locale > == > > crowdin_locale we're doing the "in supported_locales" check twice. I'm not > sure > > which is better, maybe you're right. > > Complexity-wise you have two |in supported_locales| checks either way. But with > the current implementation you are duplicating |required_locales[locale] =| as > well and have an unnecessary loop control instruction, making the code flow > harder to follow. And performance-wise, set lookups have constant time. Not that > performance matters here anyway. Done.
https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.p... cms/bin/translate.py:143: skipped_locales.append(locale) This code past hasn't been tested, obviously. ;) Hint: set object have no "append" method. https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.p... cms/bin/translate.py:269: with archive.open(member) as f, open(output_path, "wb") as f_target: Nit: Please use consistent and more descriptive variable names, source_file and target_file seems reasonable.
https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.p... cms/bin/translate.py:143: skipped_locales.append(locale) On 2015/09/17 11:03:24, Sebastian Noack wrote: > This code past hasn't been tested, obviously. ;) > > Hint: set object have no "append" method. Never mind. I just realized you turned it into a list. But why don't you just log the warning here, instead adding additional complexity to accumulate them?
Patch Set 4 : Addressed nits https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.py File cms/bin/translate.py (right): https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.p... cms/bin/translate.py:143: skipped_locales.append(locale) On 2015/09/17 12:42:15, Sebastian Noack wrote: > On 2015/09/17 11:03:24, Sebastian Noack wrote: > > This code past hasn't been tested, obviously. ;) > > > > Hint: set object have no "append" method. > > Never mind. I just realized you turned it into a list. But why don't you just > log the warning here, instead adding additional complexity to accumulate them? Done. https://codereview.adblockplus.org/29328085/diff/29328153/cms/bin/translate.p... cms/bin/translate.py:269: with archive.open(member) as f, open(output_path, "wb") as f_target: On 2015/09/17 11:03:24, Sebastian Noack wrote: > Nit: Please use consistent and more descriptive variable names, source_file and > target_file seems reasonable. Done.
LGTM 
|
