Issue 29348650: Issue 3199 - "Firefox" string is displayed on several error messages (Closed)
|
Created:
July 26, 2016, 1:31 a.m. by diegocarloslima Modified:
Jan. 2, 2017, 10:45 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 3199 - "Firefox" string is displayed on several error messages
Sibling review for this issue: https://codereview.adblockplus.org/29341317/
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed unrelated changes and added 2 more entity exceptions #
Total comments: 10
Patch Set 3 : Some readability improvements according to Felix's suggestions #
Total comments: 1
Patch Set 4 : Fixing typo #MessagesTotal messages: 9
https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... mozharness/abb/transform_locales.py:22: _SHORTNAME_RE = re.compile("^<ShortName>(.*)</ShortName>$") Moving this line around is a unrelated change. https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... mozharness/abb/transform_locales.py:94: # Fuzzy finds needle in engine_ids and returns ShortName Why do you change a docstring into comment here? Same below.
On 2016/07/26 12:49:32, René Jeschke wrote: > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > File mozharness/abb/transform_locales.py (right): > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > mozharness/abb/transform_locales.py:22: _SHORTNAME_RE = > re.compile("^<ShortName>(.*)</ShortName>$") > Moving this line around is a unrelated change. > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > mozharness/abb/transform_locales.py:94: # Fuzzy finds needle in engine_ids and > returns ShortName > Why do you change a docstring into comment here? Same below. Any updates here? Rene is not reviewer any more but i'd prefer to take his comments into account too
On 2016/09/30 06:52:24, anton wrote: > On 2016/07/26 12:49:32, René Jeschke wrote: > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > File mozharness/abb/transform_locales.py (right): > > > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:22: _SHORTNAME_RE = > > re.compile("^<ShortName>(.*)</ShortName>$") > > Moving this line around is a unrelated change. > > > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:94: # Fuzzy finds needle in engine_ids and > > returns ShortName > > Why do you change a docstring into comment here? Same below. > > Any updates here? Rene is not reviewer any more but i'd prefer to take his > comments into account too I have remove the unrelated changes in patch set 2
On 2016/10/21 14:28:53, diegocarloslima wrote: > On 2016/09/30 06:52:24, anton wrote: > > On 2016/07/26 12:49:32, René Jeschke wrote: > > > > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > > File mozharness/abb/transform_locales.py (right): > > > > > > > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > > mozharness/abb/transform_locales.py:22: _SHORTNAME_RE = > > > re.compile("^<ShortName>(.*)</ShortName>$") > > > Moving this line around is a unrelated change. > > > > > > > > > https://codereview.adblockplus.org/29348650/diff/29348651/mozharness/abb/tran... > > > mozharness/abb/transform_locales.py:94: # Fuzzy finds needle in engine_ids > and > > > returns ShortName > > > Why do you change a docstring into comment here? Same below. > > > > Any updates here? Rene is not reviewer any more but i'd prefer to take his > > comments into account too > > I have remove the unrelated changes in patch set 2 LGTM
https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:20: _VALUES_LOCALE_RE = re.compile("^values-([a-z]{2,3}(?:-r[A-Z]{2})?)$") What's the "r" for here? https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:61: _ENTITY_EXCEPTIONS = [ Why do we exclude these? Would be helpful to have in a comment. https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:103: def _replace_str_re(str, old, new, replacement_re, exceptions=[]): Took me a little while to wrap my head around what this really does. I have some ideas for making it easier to understand: 1. Find a name that describes what it does more accurately. My best bet is `_replace_in_value`, but I'm not fully convinced that's much better. 2. Rename the `replacement_re` parameter to something along the lines of `format_re`. I would also move this to be the first parameter, since it strongly affects what the function does. Likewise, I would rename `_PROPERTY_RE` and so on above to `_PROPERTY_FORMAT_RE` or something to that effect. 3. Assign temp variables for the various matched groups below, e.g. `key` for `match.group(2)`. But I guess this is least important, not that hard to get behind. https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:106: if match.group(2) not in exceptions and old in match.group(3): Shouldn't `old` be `in match.group(2)` rather than `in match.group(3)`? https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:207: """Replaces ocurrences of 'Firefox' by 'Adblock Browser' Shouldn't this be a normal comment rather than a doc string?
https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:20: _VALUES_LOCALE_RE = re.compile("^values-([a-z]{2,3}(?:-r[A-Z]{2})?)$") On 2016/12/13 13:06:05, Felix Dahlke wrote: > What's the "r" for here? Whoops, I was planning to remove this comment once I figured this out :P I did, that seems to be just the format of those values- dirs. Guess we should just roll with it.
https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:61: _ENTITY_EXCEPTIONS = [ On 2016/12/13 13:06:05, Felix Dahlke wrote: > Why do we exclude these? Would be helpful to have in a comment. These are excluded because they relate to some features that must have Firefox on it's name like Firefox Sync / Firefox Account. It wouldn't make sense to have them renamed to Adblock Browser Sync/Account. But I agree, I should add a comment to make it more clear https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:103: def _replace_str_re(str, old, new, replacement_re, exceptions=[]): On 2016/12/13 13:06:06, Felix Dahlke wrote: > Took me a little while to wrap my head around what this really does. I have some > ideas for making it easier to understand: > > 1. Find a name that describes what it does more accurately. My best bet is > `_replace_in_value`, but I'm not fully convinced that's much better. > > 2. Rename the `replacement_re` parameter to something along the lines of > `format_re`. I would also move this to be the first parameter, since it strongly > affects what the function does. Likewise, I would rename `_PROPERTY_RE` and so > on above to `_PROPERTY_FORMAT_RE` or something to that effect. > > 3. Assign temp variables for the various matched groups below, e.g. `key` for > `match.group(2)`. But I guess this is least important, not that hard to get > behind. Acknowledged. https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:106: if match.group(2) not in exceptions and old in match.group(3): On 2016/12/13 13:06:05, Felix Dahlke wrote: > Shouldn't `old` be `in match.group(2)` rather than `in match.group(3)`? Actually match.group(2) matches the id/key of the property/entity/string while match.group(3) matches its value. I will extract them as variables as you suggested to make it more clear https://codereview.adblockplus.org/29348650/diff/29348867/mozharness/abb/tran... mozharness/abb/transform_locales.py:207: """Replaces ocurrences of 'Firefox' by 'Adblock Browser' On 2016/12/13 13:06:06, Felix Dahlke wrote: > Shouldn't this be a normal comment rather than a doc string? Actually, I had to make it a multiline comment to be in compliance with PEP8 line size constraint. But I just saw that they recommend to use consecutive single line comments instead. I'll fix that
LGTM, aside from a grammer nit :P https://codereview.adblockplus.org/29348650/diff/29369542/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29348650/diff/29369542/mozharness/abb/tran... mozharness/abb/transform_locales.py:62: # Some string values that contains Firefox such as 'Firefox Sync' shouldn't be Nit: s/contains/contain/ 
|
