Issue 104 - added checktranslations function

localeTools.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import re
 import os
 import sys
 import codecs
 import json
 import urlparse
@@ skipping 284 matching lines @@
         baseDir = os.path.join(localeConfig['base_path'], dir)
         if not os.path.exists(baseDir):
             continue
         for file in os.listdir(baseDir):
             path = os.path.join(baseDir, file)
             valid_extension = file.endswith('.json')
             if os.path.isfile(path) and valid_extension and not file in files:
                 os.remove(path)
 
 
-def check_translations(dir):

[tlucas, 2017/10/25 15:09:23]

The parameter is not used anywhere in this function.

[erick, 2017/10/29 03:45:56]

Acknowledged.

[erick, 2017/10/31 04:30:00]

Done.

[erick, 2017/10/31 04:30:01]

Done.

-    for locale in os.listdir('_locales/'):

[tlucas, 2017/10/25 15:09:23]

"_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.

[erick, 2017/10/29 03:45:55]

Acknowledged.

[erick, 2017/10/31 04:29:59]

Done.

-        jsonpath = os.path.join('_locales/', locale, 'messages.json')
+def check_translations(localeConfig):
+    base_path = localeConfig['base_path']
+    for locale in os.listdir(base_path):
+        jsonpath = os.path.join(base_path, locale, 'messages.json')
 
         if not os.path.exists(jsonpath):
             continue
 
         with codecs.open(jsonpath, 'rb', encoding='utf-8') as f:
             try:
                 data = json.load(f)
-            except ValueError, e:
-                print >>sys.stderr, '%s -> File is not valid. %s' % e

[tlucas, 2017/10/25 15:09:23]

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."

[erick, 2017/10/29 03:45:55]

Acknowledged.

[tlucas, 2017/10/29 23:10:03]

This is NOT done.

[erick, 2017/10/31 04:30:01]

Done.

-
-        for key, value in data.iteritems():
-            variables = set()
-            max_length = value.get('maxLength')
-            msg_length = len(value['message'])
-            if (max_length is not None

[tlucas, 2017/10/25 15:09:23]

nit: I still suggest using a shorter variable name for "msg_length", so that the
if conditions can fit in one line.

[erick, 2017/10/29 03:45:56]

Acknowledged.

[erick, 2017/10/31 04:30:00]

Done.

[erick, 2017/10/31 04:30:01]

Done.

-                and msg_length > max_length

[tlucas, 2017/10/25 15:09:23]

Tox fails on this line:

./localeTools.py:323:17: W503 line break before binary operator

[erick, 2017/10/29 03:45:55]

Acknowledged.

[erick, 2017/10/31 04:29:59]

Done.

[erick, 2017/10/31 04:29:59]

Done.

-                or msg_length > 160):

[tlucas, 2017/10/25 15:09:23]

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

[erick, 2017/10/29 03:45:55]

Acknowledged.

[erick, 2017/10/31 04:30:00]

Done.

[erick, 2017/10/31 04:30:00]

Done.

-                print >>sys.stderr, '%s %s ->' % (locale, key), \

[tlucas, 2017/10/25 15:09:23]

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

[erick, 2017/10/29 03:45:55]

Acknowledged.

[erick, 2017/10/31 04:30:00]

Done.

[erick, 2017/10/31 04:30:01]

Done.

-                    'translation might be too long. More then', \
-                    '160 chars or maxLength exceeded'
-
-            for match in re.finditer(r'\$(\S+?)\$', value['message']):
-                variables.add(match.group(1))
-            expected = set(value.get('placeholders', {}).iterkeys())

[tlucas, 2017/10/25 15:09:23]

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.

[erick, 2017/10/29 03:45:55]

Is a possible way to check for an empty set() then go through the
default-language json?

[tlucas, 2017/10/29 23:10:03]

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)

-            if variables != expected:
-                print >>sys.stderr, '%s %s: Variables' % (locale, key), \
-                    'used are %s, expected.' % variables, \
-                    'Variables: %s' % expected
+                for key, value in data.iteritems():
+                    variables = set()
+                    max_length = value.get('maxLength')
+                    length = len(value['message'])
+                    if (max_length is not None and
+                            length > max_length or length > 160):
+                        print >>sys.stderr, (
+                            '{} {} -> translation might be too long.'
+                            'More then 160 chars or maxLength exceeded'
+                        ).format(locale, key)
+
+                    for match in re.finditer(r'\$(\S+?)\$', value['message']):
+                        variables.add(match.group(1))
+                    expected = set(value.get('placeholders', {}).iterkeys())
+                    if variables != expected:
+                        print >>sys.stderr, (
+                            '{} {}: Variables used are {} '
+                            'expected. Variables: {}'
+                        ).format(locale, key, variables, expected)
+            except ValueError:
+                print >>sys.stderr, locale + ', messages.json is not valid.'