Issue 29587910: Issue 104 - added checktranslations function
DescriptionIssue 104 - added checktranslations function
Patch Set 1 #
Total comments: 34
Patch Set 2 : Proposed changes #
Total comments: 8
Patch Set 3 : Proposed changes #
Total comments: 2
Patch Set 4 : Proposed changes #
Total comments: 2
Patch Set 5 : Proposed changes #
Total comments: 2
Patch Set 6 : Proposed changes #Patch Set 7 : Proposed changes #MessagesTotal messages: 14
Hi Erick! Here you are (for starters) - it's almost there, but nit quite. Keep up the work ;) Cheers! https://codereview.adblockplus.org/29587910/diff/29587911/build.py File build.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/build.py#newcode449 build.py:449: command.shortDescription = 'Checks translation files for overlong, missing and structure' nit: this line exceeds 79 characters nit: "Checks translation files for overlong, missing and structure" -> maybe "Checks translation files for structure and for overlong and missing translations" https://codereview.adblockplus.org/29587910/diff/29587911/build.py#newcode450 build.py:450: command.description = 'JSON files translations will be checked by this functionality' nit: this line exceeds 79 characters nit: description is shorter than shortDescription? nit: Maybe something like "Perform translation checks" and switch this with shortDescription https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:305: def check_translations(dir): The parameter is not used anywhere in this function. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:306: for locale in os.listdir('_locales/'): "_locales" is only valid for adblockpluschrome. in adblockplusui it's "locale". The proper solution would be to parse the target's metadata and determine the locale-path by reading "base_path" from the "locales" section. Hint: this is already done in "readLocaleConfig" in build.py Additional Hint: "readLocaleConfig" also provides you with a list of available locales. Please use it. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:316: print >>sys.stderr, '%s -> File is not valid. %s' % e nit: https://adblockplus.org/en/coding-style (section "Python"): "Use the + operator when concatenating exactly two strings, use the format() method for more complex string formatting, use the join() method when concatenating pre-existing sequences." https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:322: if (max_length is not None nit: I still suggest using a shorter variable name for "msg_length", so that the if conditions can fit in one line. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:323: and msg_length > max_length Tox fails on this line: ./localeTools.py:323:17: W503 line break before binary operator https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:324: or msg_length > 160): Tox fails on this line: ./localeTools.py:324:17: W503 line break before binary operator ./localeTools.py:324:17: E129 visually indented line with same indent as next logical line https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:325: print >>sys.stderr, '%s %s ->' % (locale, key), \ https://adblockplus.org/en/coding-style (section "Python"): "Use the + operator when concatenating exactly two strings, use the format() method for more complex string formatting, use the join() method when concatenating pre-existing sequences." -> don't use "foo%s" % ('bar') Additionally, don't use "\" to enable line continuation. Make use of parentheses, that way python will simply concatenate multi-line strings itself: print >>sys.stderr, ( '{} {} -> translation might be too long. More than 160 ' 'chars or maxLength exceeded' ).format(locale, key) for both: same below https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:331: expected = set(value.get('placeholders', {}).iterkeys()) The above 3 lines seem rather complicated. Maybe something like this would be more readable: if not all([placeholder in value['messages'] for placeholder in expected]): ... Addtionally, you are relying on the key/value pairs "placeholder" to be present in every messages.json - are we sure that this is given? If not, i suggest reading the placeholder from the default-language messages.json.
https://codereview.adblockplus.org/29587910/diff/29587911/build.py File build.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/build.py#newcode449 build.py:449: command.shortDescription = 'Checks translation files for overlong, missing and structure' On 2017/10/25 15:09:23, tlucas wrote: > nit: this line exceeds 79 characters > nit: "Checks translation files for overlong, missing and structure" -> maybe > "Checks translation files for structure and for overlong and missing > translations" Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/build.py#newcode450 build.py:450: command.description = 'JSON files translations will be checked by this functionality' On 2017/10/25 15:09:23, tlucas wrote: > nit: this line exceeds 79 characters > nit: description is shorter than shortDescription? > nit: Maybe something like "Perform translation checks" and switch this with > shortDescription Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:305: def check_translations(dir): On 2017/10/25 15:09:23, tlucas wrote: > The parameter is not used anywhere in this function. Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:306: for locale in os.listdir('_locales/'): On 2017/10/25 15:09:23, tlucas wrote: > "_locales" is only valid for adblockpluschrome. > > in adblockplusui it's "locale". > > The proper solution would be to parse the target's metadata and determine the > locale-path by reading "base_path" from the "locales" section. > > Hint: this is already done in "readLocaleConfig" in build.py > Additional Hint: "readLocaleConfig" also provides you with a list of available > locales. > > Please use it. Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:316: print >>sys.stderr, '%s -> File is not valid. %s' % e On 2017/10/25 15:09:23, tlucas wrote: > nit: https://adblockplus.org/en/coding-style (section "Python"): > > "Use the + operator when concatenating exactly two strings, use the format() > method for more complex string formatting, use the join() method when > concatenating pre-existing sequences." Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:322: if (max_length is not None On 2017/10/25 15:09:23, tlucas wrote: > nit: I still suggest using a shorter variable name for "msg_length", so that the > if conditions can fit in one line. Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:323: and msg_length > max_length On 2017/10/25 15:09:23, tlucas wrote: > Tox fails on this line: > > ./localeTools.py:323:17: W503 line break before binary operator Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:324: or msg_length > 160): On 2017/10/25 15:09:23, tlucas wrote: > Tox fails on this line: > > ./localeTools.py:324:17: W503 line break before binary operator > ./localeTools.py:324:17: E129 visually indented line with same indent as next > logical line Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:325: print >>sys.stderr, '%s %s ->' % (locale, key), \ On 2017/10/25 15:09:23, tlucas wrote: > https://adblockplus.org/en/coding-style (section "Python"): > > "Use the + operator when concatenating exactly two strings, use the format() > method for more complex string formatting, use the join() method when > concatenating pre-existing sequences." > > -> don't use "foo%s" % ('bar') > > Additionally, don't use "\" to enable line continuation. Make use of > parentheses, that way python will simply concatenate multi-line strings itself: > > print >>sys.stderr, ( > '{} {} -> translation might be too long. More than 160 ' > 'chars or maxLength exceeded' > ).format(locale, key) > > > for both: same below Acknowledged. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:331: expected = set(value.get('placeholders', {}).iterkeys()) On 2017/10/25 15:09:23, tlucas wrote: > The above 3 lines seem rather complicated. Maybe something like this would be > more readable: > > if not all([placeholder in value['messages'] for placeholder in expected]): > ... > > Addtionally, you are relying on the key/value pairs "placeholder" to be present > in every messages.json - are we sure that this is given? If not, i suggest > reading the placeholder from the default-language messages.json. Is a possible way to check for an empty set() then go through the default-language json?
Hey Erick! A couple more comments (in both Patch Sets, please watch out for both). Cheers, Tristan https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:316: print >>sys.stderr, '%s -> File is not valid. %s' % e On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > nit: https://adblockplus.org/en/coding-style (section "Python"): > > > > "Use the + operator when concatenating exactly two strings, use the format() > > method for more complex string formatting, use the join() method when > > concatenating pre-existing sequences." > > Acknowledged. This is NOT done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:331: expected = set(value.get('placeholders', {}).iterkeys()) On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > The above 3 lines seem rather complicated. Maybe something like this would be > > more readable: > > > > if not all([placeholder in value['messages'] for placeholder in expected]): > > ... > > > > Addtionally, you are relying on the key/value pairs "placeholder" to be > present > > in every messages.json - are we sure that this is given? If not, i suggest > > reading the placeholder from the default-language messages.json. > > Is a possible way to check for an empty set() then go through the > default-language json? Why would you want to potentially double cpu load? Cycling through the placeholders expected in the default-language is perfectly sufficient. Technically: yes, but i strongly discourage your approach (if i got it right) https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:307: json_fld = path.rsplit('/', 1)[1] Use os.path.split() to split paths. a) This will succeed, no matter what operating system the user is running b) The behavior of os.path.plit('/foo/bar/baz/') -> ['/foo/bar', 'baz'] is exactly what you are trying to accomplish here. https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:308: for locale in os.listdir('_locales/'): As pointed in out in the last comment '_locales' is not universal. Additionally, you are introducing json_fld, but your are not using it anywhere. your variable "path" should be your starting point - plaase consult https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l214 (and surrounding) for an example on how to validly determine locale filenames. https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:333: print >>sys.stderr, ( nit: this is rather awkward to read. maybe something like ... ... Variables: {}' ).format(locale, key, variables, expected) ?
https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:305: def check_translations(dir): On 2017/10/29 03:45:56, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > The parameter is not used anywhere in this function. > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:305: def check_translations(dir): On 2017/10/29 03:45:56, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > The parameter is not used anywhere in this function. > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:306: for locale in os.listdir('_locales/'): On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > "_locales" is only valid for adblockpluschrome. > > > > in adblockplusui it's "locale". > > > > The proper solution would be to parse the target's metadata and determine the > > locale-path by reading "base_path" from the "locales" section. > > > > Hint: this is already done in "readLocaleConfig" in build.py > > Additional Hint: "readLocaleConfig" also provides you with a list of available > > locales. > > > > Please use it. > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:316: print >>sys.stderr, '%s -> File is not valid. %s' % e On 2017/10/29 23:10:03, tlucas wrote: > On 2017/10/29 03:45:55, erick wrote: > > On 2017/10/25 15:09:23, tlucas wrote: > > > nit: https://adblockplus.org/en/coding-style (section "Python"): > > > > > > "Use the + operator when concatenating exactly two strings, use the format() > > > method for more complex string formatting, use the join() method when > > > concatenating pre-existing sequences." > > > > Acknowledged. > > This is NOT done. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:322: if (max_length is not None On 2017/10/25 15:09:23, tlucas wrote: > nit: I still suggest using a shorter variable name for "msg_length", so that the > if conditions can fit in one line. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:322: if (max_length is not None On 2017/10/25 15:09:23, tlucas wrote: > nit: I still suggest using a shorter variable name for "msg_length", so that the > if conditions can fit in one line. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:323: and msg_length > max_length On 2017/10/25 15:09:23, tlucas wrote: > Tox fails on this line: > > ./localeTools.py:323:17: W503 line break before binary operator Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:323: and msg_length > max_length On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > Tox fails on this line: > > > > ./localeTools.py:323:17: W503 line break before binary operator > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:324: or msg_length > 160): On 2017/10/25 15:09:23, tlucas wrote: > Tox fails on this line: > > ./localeTools.py:324:17: W503 line break before binary operator > ./localeTools.py:324:17: E129 visually indented line with same indent as next > logical line Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:324: or msg_length > 160): On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > Tox fails on this line: > > > > ./localeTools.py:324:17: W503 line break before binary operator > > ./localeTools.py:324:17: E129 visually indented line with same indent as next > > logical line > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:325: print >>sys.stderr, '%s %s ->' % (locale, key), \ On 2017/10/25 15:09:23, tlucas wrote: > https://adblockplus.org/en/coding-style (section "Python"): > > "Use the + operator when concatenating exactly two strings, use the format() > method for more complex string formatting, use the join() method when > concatenating pre-existing sequences." > > -> don't use "foo%s" % ('bar') > > Additionally, don't use "\" to enable line continuation. Make use of > parentheses, that way python will simply concatenate multi-line strings itself: > > print >>sys.stderr, ( > '{} {} -> translation might be too long. More than 160 ' > 'chars or maxLength exceeded' > ).format(locale, key) > > > for both: same below Done. https://codereview.adblockplus.org/29587910/diff/29587911/localeTools.py#newc... localeTools.py:325: print >>sys.stderr, '%s %s ->' % (locale, key), \ On 2017/10/29 03:45:55, erick wrote: > On 2017/10/25 15:09:23, tlucas wrote: > > https://adblockplus.org/en/coding-style (section "Python"): > > > > "Use the + operator when concatenating exactly two strings, use the format() > > method for more complex string formatting, use the join() method when > > concatenating pre-existing sequences." > > > > -> don't use "foo%s" % ('bar') > > > > Additionally, don't use "\" to enable line continuation. Make use of > > parentheses, that way python will simply concatenate multi-line strings > itself: > > > > print >>sys.stderr, ( > > '{} {} -> translation might be too long. More than 160 ' > > 'chars or maxLength exceeded' > > ).format(locale, key) > > > > > > for both: same below > > Acknowledged. Done. https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:307: json_fld = path.rsplit('/', 1)[1] On 2017/10/29 23:10:04, tlucas wrote: > Use os.path.split() to split paths. > > a) This will succeed, no matter what operating system the user is running > b) The behavior of os.path.plit('/foo/bar/baz/') -> ['/foo/bar', 'baz'] is > exactly what you are trying to accomplish here. I receive "AttributeError: 'tuple' object has no attribute 'endswith'", when I can't use os.path.split() and var.rsplit() is not sufficient, do I have other options? The problem came up, when I try to join the strings to the path together. Line 309 https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:308: for locale in os.listdir('_locales/'): On 2017/10/29 23:10:04, tlucas wrote: > As pointed in out in the last comment '_locales' is not universal. Additionally, > you are introducing json_fld, but your are not using it anywhere. > > your variable "path" should be your starting point - > > plaase consult > https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l214 (and > surrounding) for an example on how to validly determine locale filenames. Done. https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:333: print >>sys.stderr, ( On 2017/10/29 23:10:04, tlucas wrote: > nit: this is rather awkward to read. > > maybe something like > > ... > ... Variables: {}' > ).format(locale, key, variables, expected) > > ? Done.
https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:307: json_fld = path.rsplit('/', 1)[1] On 2017/10/31 04:30:02, erick wrote: > On 2017/10/29 23:10:04, tlucas wrote: > > Use os.path.split() to split paths. > > > > a) This will succeed, no matter what operating system the user is running > > b) The behavior of os.path.plit('/foo/bar/baz/') -> ['/foo/bar', 'baz'] is > > exactly what you are trying to accomplish here. > > I receive "AttributeError: 'tuple' object has no attribute 'endswith'", when I > can't use os.path.split() and var.rsplit() is not sufficient, do I have other > options? The problem came up, when I try to join the strings to the path > together. Line 309 The error message pointed you to passing a tuple, instead of string. I guess you forgot the access operator here? However: you do NOT need to split anything, since base_path + locale already is the correct folder, am i right? https://codereview.adblockplus.org/29587910/diff/29592652/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29592652/localeTools.py#newc... localeTools.py:318: print >>sys.stderr, '%s -> File is not valid.' + e ? Does this even work? I am expecting either .format() - or concatenating with "+", with the strings in the correct order and without placeholders.
https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29590632/localeTools.py#newc... localeTools.py:307: json_fld = path.rsplit('/', 1)[1] On 2017/10/31 07:43:09, tlucas wrote: > On 2017/10/31 04:30:02, erick wrote: > > On 2017/10/29 23:10:04, tlucas wrote: > > > Use os.path.split() to split paths. > > > > > > a) This will succeed, no matter what operating system the user is running > > > b) The behavior of os.path.plit('/foo/bar/baz/') -> ['/foo/bar', 'baz'] is > > > exactly what you are trying to accomplish here. > > > > I receive "AttributeError: 'tuple' object has no attribute 'endswith'", when I > > can't use os.path.split() and var.rsplit() is not sufficient, do I have other > > options? The problem came up, when I try to join the strings to the path > > together. Line 309 > > The error message pointed you to passing a tuple, instead of string. I guess you > forgot the access operator here? > > However: you do NOT need to split anything, since base_path + locale already is > the correct folder, am i right? Done. https://codereview.adblockplus.org/29587910/diff/29592652/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29592652/localeTools.py#newc... localeTools.py:318: print >>sys.stderr, '%s -> File is not valid.' + e On 2017/10/31 07:43:10, tlucas wrote: > ? Does this even work? > I am expecting either .format() - or concatenating with "+", with the strings in > the correct order and without placeholders. Done.
Hey Erick. Please consider running tox every single time, before you upload a patch. Cheers! https://codereview.adblockplus.org/29587910/diff/29599555/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29599555/localeTools.py#newc... localeTools.py:335: ).format(locale, key, variables, expected) py27 runtests: commands[2] | flake8 ./localeTools.py:335:21: E123 closing bracket does not match indentation of opening bracket's line
https://codereview.adblockplus.org/29587910/diff/29599555/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29599555/localeTools.py#newc... localeTools.py:335: ).format(locale, key, variables, expected) On 2017/11/06 15:48:12, tlucas wrote: > py27 runtests: commands[2] | flake8 > ./localeTools.py:335:21: E123 closing bracket does not match indentation of > opening bracket's line Done.
LGTM (for the project)
https://codereview.adblockplus.org/29587910/diff/29600567/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29600567/localeTools.py#newc... localeTools.py:319: for key, value in data.iteritems(): NOT LGTM, found a late error. sorry. When a messages.json can not be read, but a previous messages.json was read successfully, the following code is executed on the translations which were already checked. You should do the additional checks inside the "try"-block above
https://codereview.adblockplus.org/29587910/diff/29600567/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29587910/diff/29600567/localeTools.py#newc... localeTools.py:319: for key, value in data.iteritems(): On 2017/11/13 12:23:51, tlucas wrote: > NOT LGTM, found a late error. sorry. > > When a messages.json can not be read, but a previous messages.json was read > successfully, the following code is executed on the translations which were > already checked. > > You should do the additional checks inside the "try"-block above Done. tox and flake8 were fine, too.
LGTM 
|
