Issue 29517660: Issue 5477 - Import everything from imported locales (Closed)
DescriptionIssue 5477 - Import everything from imported locales
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Adressing comment from Sebastian #
Total comments: 2
Patch Set 4 : Moving access key removal #
Total comments: 2
Patch Set 5 : Further simplifying #
Total comments: 10
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : #
Total comments: 12
Patch Set 9 : #Patch Set 10 : Minor docstring changes #
Total comments: 4
Patch Set 11 : #
MessagesTotal messages: 27
Patch Set 1: * Let every additional information when importing translations in packagerChrome/Safari be included * Renamed importGeckoLocales to import_locales (in order clearify it not only being responsible for Gecko-locales)
It seems you missed at least one usage of importGeckoLocales (in packagerEdge.py). Also this code doesn't seem to be used by packagerGecko.py at all. But for the gecko-webext build the same translations need to be imported in the same way, or do I miss something here? Did you test these changes with the actually scenario, i.e. by generating builds from the adblockpluschrome repository for all build targets (i.e. "chrome", "edge" and "gecko-webext"). Also make sure that all builds import strings from adblockplusui including placeholders. You have to patch the buildtools and adblockplusui dependencies locally, therefore.
Patch Set 2 On 2017/08/16 15:35:00, Sebastian Noack wrote: > It seems you missed at least one usage of importGeckoLocales (in > packagerEdge.py). Also this code doesn't seem to be used by packagerGecko.py at > all. But for the gecko-webext build the same translations need to be imported in > the same way, or do I miss something here? > > Did you test these changes with the actually scenario, i.e. by generating builds > from the adblockpluschrome repository for all build targets (i.e. "chrome", > "edge" and "gecko-webext"). Also make sure that all builds import strings from > adblockplusui including placeholders. You have to patch the buildtools and > adblockplusui dependencies locally, therefore. You are right - i missed the one in packagerEdge. for the tests: yes, i ran gecko-webext and chrome in patch-set one (according to Manvel's propsed changes) and edge in this one -> placeholders are included. As for packagerGecko -> gecko-webext does not use it for import locales (hence no error in classic gecko)
On 2017/08/16 18:33:15, tlucas wrote: > As for packagerGecko -> gecko-webext does not use it for import locales (hence > no error in classic gecko) I just noticed that gecko-webext is build by packagerChrome.py, not packagerGecko.py, which is the reason why it works. https://codereview.adblockplus.org/29517660/diff/29517743/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29517743/packagerChrome.py#n... packagerChrome.py:251: get_value = lambda x: x[stringID]['message'] PEP-8 discourages assignment of lambda functions to variables, and suggests to use def instead.This also causes an E731 with flake8, however, it might be ignored for this file, because of legacy code that already uses this practice. When turning get_value() into a proper function defined here, you also might want to define set_translation() the same way for consistency.
Patch Set 3 * Removing lambdas * Moving additional function-defs for consistency https://codereview.adblockplus.org/29517660/diff/29517743/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29517743/packagerChrome.py#n... packagerChrome.py:251: get_value = lambda x: x[stringID]['message'] On 2017/08/17 16:32:34, Sebastian Noack wrote: > PEP-8 discourages assignment of lambda functions to variables, and suggests to > use def instead.This also causes an E731 with flake8, however, it might be > ignored for this file, because of legacy code that already uses this practice. > > When turning get_value() into a proper function defined here, you also might > want to define set_translation() the same way for consistency. Done.
https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py File packagerChrome.py (left): https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py#o... packagerChrome.py:260: match = re.search(r'^(.*?)\s*\(&.\)$', value) The logic to strip the access keys here is actually specific to Gecko translations, i.e. irrelevant for .json files. IT seems we could simplify the abstraction if we only perform this code for Gecko translations.
Patch Set 4 Moving access key-removal to possible gecko-style translations only https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py File packagerChrome.py (left): https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py#o... packagerChrome.py:260: match = re.search(r'^(.*?)\s*\(&.\)$', value) On 2017/08/18 07:44:28, Sebastian Noack wrote: > The logic to strip the access keys here is actually specific to Gecko > translations, i.e. irrelevant for .json files. IT seems we could simplify the > abstraction if we only perform this code for Gecko translations. Ok - i will move this to the specific "get_value" instead -> done
https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py#n... packagerChrome.py:287: set_translation(data, key, value, sourceData, stringID) It seems the logic could be further simplified by merging get_value() and set_translation() as there is no common logic in the surrounding code anymore.
Patch Set 5 Further simplified the import_locales code, moving get_ and set_ logic to top-level as discussed https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py#n... packagerChrome.py:287: set_translation(data, key, value, sourceData, stringID) On 2017/08/18 13:54:48, Sebastian Noack wrote: > It seems the logic could be further simplified by merging get_value() and > set_translation() as there is no common logic in the surrounding code anymore. Done.
https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:195: info from the source dict""" As per PEP-257, the trailing """ goes onto a new line in multi-line docstrings. https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:196: value = source[stringID]['message'] It seems all usage of "source" and "stringID" here and in the other function are in the form of "source[stringID]". So how about passing "source[stringID]" directly to these functions rather than passing these arguments seperately and duplicating the lookup? https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:203: def set_translation_without_access_keys(data, key, source, stringID): I think better names for these functions would be import_string_webext() and import_string_gecko(). https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:258: # .json and other formats provide translations differently and The other format is the Gecko format. "The WebExtensions (.json) and Gecko format provide ..." https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:259: # / or provide additional information like e.g. "placeholders". It is supposed to be "and/or" without space.
Patch Set 6 Simplified arguments (and func-names) of importing logic, adhered to Sebastian's comments on comments and Docstrings https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:195: info from the source dict""" On 2017/08/18 14:53:59, Sebastian Noack wrote: > As per PEP-257, the trailing """ goes onto a new line in multi-line docstrings. Done. https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:196: value = source[stringID]['message'] On 2017/08/18 14:53:59, Sebastian Noack wrote: > It seems all usage of "source" and "stringID" here and in the other function are > in the form of "source[stringID]". So how about passing "source[stringID]" > directly to these functions rather than passing these arguments seperately and > duplicating the lookup? Done. https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:203: def set_translation_without_access_keys(data, key, source, stringID): On 2017/08/18 14:53:59, Sebastian Noack wrote: > I think better names for these functions would be import_string_webext() and > import_string_gecko(). Done. https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:258: # .json and other formats provide translations differently and On 2017/08/18 14:53:59, Sebastian Noack wrote: > The other format is the Gecko format. > > "The WebExtensions (.json) and Gecko format provide ..." Done. https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#n... packagerChrome.py:259: # / or provide additional information like e.g. "placeholders". On 2017/08/18 14:53:59, Sebastian Noack wrote: > It is supposed to be "and/or" without space. Done.
https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:205: """only set {'message': value} in data-dictionary, after stripping You capitalized the above docstring but not this one. Also if capatalized there should be a full-stop at the end of the sentence. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:259: # The WebExtensions (.json) and Gecko format provide There is a redundant space in this line. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:262: # that and preserve the addtional info Missing full-stop at end of sentence. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:268: set_translation = import_string_webext For consistency you might want to rename the set_translation variable to import_string as well.
Patch Set 7 * Consistency in function names * Comment- / docstring-optimization https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:205: """only set {'message': value} in data-dictionary, after stripping On 2017/08/18 15:26:14, Sebastian Noack wrote: > You capitalized the above docstring but not this one. Also if capatalized there > should be a full-stop at the end of the sentence. Done. Also added full-stop to the above docstring. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:259: # The WebExtensions (.json) and Gecko format provide On 2017/08/18 15:26:14, Sebastian Noack wrote: > There is a redundant space in this line. Done. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:262: # that and preserve the addtional info On 2017/08/18 15:26:14, Sebastian Noack wrote: > Missing full-stop at end of sentence. Done. https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#n... packagerChrome.py:268: set_translation = import_string_webext On 2017/08/18 15:26:14, Sebastian Noack wrote: > For consistency you might want to rename the set_translation variable to > import_string as well. Done.
LGTM
Patch Set 8 Sebastian figured out that import_string_webext could simply assign source to the target[key], which was not possible a few iterations ago. Creating this patch set in order to adhere to this.
LGTM
Hi Tristan, It looks good to me and I just have two stylistic comments, one kind of important, another not really. See below. Another comment: maybe we should add a test the reproduces the bug that this change is fixing although perhaps this would belong to the testing review. Cheers, Vasily https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:199: """Only sets {'message': value} in data-dictionary, after stripping Do you think it would be difficult to make this docstring (and the one above) conform to PEP-257[1]? Namely, that the first line of a docstring should be an imperative sentence (e.g. "Compute the answer.") and any follow up lines should be separated from the first line by an empty space. [1] https://www.python.org/dev/peps/pep-0257 https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:261: I don't feel very strong about it, but it seems that this empty line doesn't improve readability (like, for example the lines that separate two blocks that do different things and each consist of multiple lines). Maybe we don't need it (as well as the line 265)?
https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:203: # Remove access keys from possible gecko-style Nit: Gecko should be capitalized (here and in the docstring above). https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:205: match = re.search(r'^(.*?)\s*\(&.\)$', value) For reference, the regexp used by Firefox code is slightly different and considers potential ellipsis as well: https://hg.adblockplus.org/adblockplus/file/86f4e19aafd5/lib/utils.js#l430 Not that this is worth changing now, we are definitely not going to add more imports from Gecko at this point. So if it wasn't relevant so far, it isn't going to be in future either.
Hey guys, thank you for the comments! Vasily, please find the discussion in the docstring-comment. Best, Tristan https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:199: """Only sets {'message': value} in data-dictionary, after stripping On 2017/08/21 10:43:18, Vasily Kuznetsov wrote: > Do you think it would be difficult to make this docstring (and the one above) > conform to PEP-257[1]? Namely, that the first line of a docstring should be an > imperative sentence (e.g. "Compute the answer.") and any follow up lines should > be separated from the first line by an empty space. > > [1] https://www.python.org/dev/peps/pep-0257 Do you mean full conformity, including params? For the docstring above, from what i see it'd be sufficient to just rephrase it to e.g. "Import source-dict into data", since it's already a 1-liner - or am i missing something? https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:203: # Remove access keys from possible gecko-style On 2017/08/21 10:57:21, Wladimir Palant wrote: > Nit: Gecko should be capitalized (here and in the docstring above). Acknowledged. https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:205: match = re.search(r'^(.*?)\s*\(&.\)$', value) On 2017/08/21 10:57:21, Wladimir Palant wrote: > For reference, the regexp used by Firefox code is slightly different and > considers potential ellipsis as well: > https://hg.adblockplus.org/adblockplus/file/86f4e19aafd5/lib/utils.js#l430 > > Not that this is worth changing now, we are definitely not going to add more > imports from Gecko at this point. So if it wasn't relevant so far, it isn't > going to be in future either. Acknowledged. https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:261: On 2017/08/21 10:43:18, Vasily Kuznetsov wrote: > I don't feel very strong about it, but it seems that this empty line doesn't > improve readability (like, for example the lines that separate two blocks that > do different things and each consist of multiple lines). Maybe we don't need it > (as well as the line 265)? Acknowledged.
https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:199: """Only sets {'message': value} in data-dictionary, after stripping On 2017/08/21 11:33:01, tlucas wrote: > On 2017/08/21 10:43:18, Vasily Kuznetsov wrote: > > Do you think it would be difficult to make this docstring (and the one above) > > conform to PEP-257[1]? Namely, that the first line of a docstring should be an > > imperative sentence (e.g. "Compute the answer.") and any follow up lines > should > > be separated from the first line by an empty space. > > > > [1] https://www.python.org/dev/peps/pep-0257 > > Do you mean full conformity, including params? > For the docstring above, from what i see it'd be sufficient to just rephrase it > to e.g. "Import source-dict into data", since it's already a 1-liner - or am i > missing something? Yeah, just rephrase and reformat where appropriate. I don't think you need to add the parameters unless you consider them useful and would like to add them, PEP-257 doesn't require any of that.
Patch Set 9 * removed unnecessary newlines * Edited docstrings according to PEP-257 https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:199: """Only sets {'message': value} in data-dictionary, after stripping On 2017/08/21 11:53:15, Vasily Kuznetsov wrote: > On 2017/08/21 11:33:01, tlucas wrote: > > On 2017/08/21 10:43:18, Vasily Kuznetsov wrote: > > > Do you think it would be difficult to make this docstring (and the one > above) > > > conform to PEP-257[1]? Namely, that the first line of a docstring should be > an > > > imperative sentence (e.g. "Compute the answer.") and any follow up lines > > should > > > be separated from the first line by an empty space. > > > > > > [1] https://www.python.org/dev/peps/pep-0257 > > > > Do you mean full conformity, including params? > > For the docstring above, from what i see it'd be sufficient to just rephrase > it > > to e.g. "Import source-dict into data", since it's already a 1-liner - or am i > > missing something? > > Yeah, just rephrase and reformat where appropriate. I don't think you need to > add the parameters unless you consider them useful and would like to add them, > PEP-257 doesn't require any of that. Done. https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:203: # Remove access keys from possible gecko-style On 2017/08/21 11:33:00, tlucas wrote: > On 2017/08/21 10:57:21, Wladimir Palant wrote: > > Nit: Gecko should be capitalized (here and in the docstring above). > > Acknowledged. Done. https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#n... packagerChrome.py:261: On 2017/08/21 11:33:01, tlucas wrote: > On 2017/08/21 10:43:18, Vasily Kuznetsov wrote: > > I don't feel very strong about it, but it seems that this empty line doesn't > > improve readability (like, for example the lines that separate two blocks that > > do different things and each consist of multiple lines). Maybe we don't need > it > > (as well as the line 265)? > > Acknowledged. Done.
Patch Set 10 Changed docstrings to be imperative with a full-stop.
LGTM (one nit below) https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#n... packagerChrome.py:194: """Import source-dict into data.""" Nit: "Import a single translation from source dictionary into data"?
https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#n... packagerChrome.py:256: Nit: The blank line here seems out of place.
Patch Set 11 Rephrase a doscstring / remove a newline https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#n... packagerChrome.py:194: """Import source-dict into data.""" On 2017/08/21 13:59:05, Wladimir Palant wrote: > Nit: "Import a single translation from source dictionary into data"? Done. https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#n... packagerChrome.py:256: On 2017/08/22 07:38:11, Sebastian Noack wrote: > Nit: The blank line here seems out of place. Done.
LGTM
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
