Issue 2625 - [cms] Crowdin synchronisation script

cms/bin/translate.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
-from io import BytesIO
-from itertools import islice

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: I'm not a big fan of importing individual functions, other than in cases
where function name and module name are identical - when looking at a function
call, it becomes less obvious that the function is imported from somewhere else
as opposed to being defined in this file. Writing io.BytesIO() isn't much longer
than BytesIO() but makes the code more obvious.

[kzar, 2015/07/02 12:33:11]

Done.

+import collections
+import io
+import itertools
 import json
 import logging
 import os
+import posixpath
 import sys
+import urllib
 import zipfile
 
-import requests
+import urllib3
 
 import cms.utils
 from cms.sources import FileSource
 
-FILES_PER_REQUEST = 20
-
 logger = logging.getLogger("cms.bin.translate")
-crowdin_api_key = None
-crowdin_project_name = None
-defaultlocale = None

[Wladimir Palant, 2015/06/29 19:05:37]

The three variables above shouldn't be globals - they refer to a specific
invocation of crowdin_sync and should be passed in a parameters wherever
appropriate.

Note that you can encapsulate API key and project name in a CrowdinAPI class -
then you only need to pass in an instance of that class, and the functions can
call api.GET("info") for example without any extra parameters.

[kzar, 2015/07/02 12:33:11]

Done.

+
+class CrowdinAPI:
+  FILES_PER_REQUEST = 20
+
+  def __init__(self, api_key, project_name):
+    self.api_key = api_key
+    self.project_name = project_name
+    self.connection = urllib3.connection_from_url("https://api.crowdin.com/")
+
+  def raw_request(self, request_method, api_endpoint, query_params=(), **kwargs):
+    url = "/api/project/%s/%s?%s" % (
+      urllib.quote(self.project_name),
+      urllib.quote(api_endpoint),
+      urllib.urlencode((("key", self.api_key),) + query_params)
+    )
+    try:
+      response = self.connection.request(
+        request_method, str(url), **kwargs
+      )
+    except urllib3.exceptions.HTTPError:
+      logger.error("Connection to API endpoint %s failed", url)
+      raise
+    if response.status < 200 or response.status >= 300:
+      logger.error("API call to %s failed:\n%s", url, response.data)
+      raise urllib3.exceptions.HTTPError(response.status)
+    return response
+
+  def request(self, request_method, api_endpoint, data=None, files=None):
+    fields = []
+    if data:
+      for name, value in data.iteritems():
+        if isinstance(value, basestring):
+          fields.append((name, value))
+        else:
+          fields.extend((name + "[]", v) for v in value)
+    if files:
+      fields.extend(("files[%s]" % f[0], f) for f in files)
+
+    response = self.raw_request(
+      request_method, api_endpoint, (("json", "1"),),
+      fields=fields, preload_content=False
+    )
+
+    try:
+      return json.load(response)
+    except ValueError:
+      logger.error("Invalid response returned by API endpoint %s", url)
+      raise
+
 
 def grouper(iterable, n):
   iterator = iter(iterable)
   while True:
-    chunk = tuple(islice(iterator, n))
-    if chunk:
-      yield chunk
-    else:
+    chunk = tuple(itertools.islice(iterator, n))
+    if not chunk:
       break

[Wladimir Palant, 2015/06/29 19:05:36]

Please link to
https://stackoverflow.com/questions/3992735/python-generator-that-groups-anot...
which is where you've got that code it seems.

IMPORTANT: *Always* indicate code that you didn't write yourself.

[kzar, 2015/07/02 12:33:10]

I originally did take this function from Stackoverflow and I added a comment to
indicate that
https://github.com/kzar/cms/commit/4196784#diff-d9a5accf75fa7c66e418d6e9b1c78...
. Since then I was unhappy with the implementation and the others I found on
Stackoverflow and I wrote this one myself, mixing together parts from various
examples.

I was unsure weather to leave the comment in or not before posting the review as
I was no longer left with code that exactly matched any of the examples. I
decided to leave it out and I should have mentioned that in the review, sorry.
I'm happy to either add or leave the comment out, I agree it's very important to
give proper attribution.

-
-def crowdin_request(request_method, api_endpoint, **kwargs):
-  url = "https://api.crowdin.com/api/project/%s/%s?key=%s&json=1" % (
-    crowdin_project_name, api_endpoint, crowdin_api_key
-  )
-  response = requests.request(request_method, url, **kwargs)

[Wladimir Palant, 2015/06/29 19:05:36]

You seem to be assuming that this will not throw an exception. However,
HTTPError is about HTTP response codes like 404, but there is also
ConnectionError and the like - e.g. if the host name could not be resolved. I'm
pretty sure that those will be raised when requests.request() is called.

[kzar, 2015/07/02 12:33:12]

Done.

-  try:
-    response.raise_for_status()
-  except requests.exceptions.HTTPError as e:
-    logger.error("API call to %s failed:\n%s" % (url, response.text))
-    raise
-  return response.json()

[Wladimir Palant, 2015/06/29 19:05:37]

This call might also raise an exception - if JSON decoding fails. You need to
intercept it as well if you want to log the error consistently.

[kzar, 2015/07/02 12:33:12]

Done.

+    yield chunk
 
 def extract_strings(source, defaultlocale):
   logger.info("Extracting page strings (please be patient)...")
   page_strings = {}
+
+  def record_string(page, locale, name, value, comment, fixed_strings):
+    if locale != defaultlocale:
+      return
+
+    try:
+      store = page_strings[page]
+    except KeyError:
+      store = page_strings[page] = collections.OrderedDict()
+
+    store[name] = {"message": value}
+
+    if fixed_strings:
+      comment = comment + "\n" if comment else ""
+      comment += ", ".join("{%d}: %s" % i_s
+                           for i_s in enumerate(fixed_strings, 1))
+    if comment:
+      store[name]["description"] = comment
+
   for page, format in source.list_pages():
-    params = cms.utils.get_page_params(source, defaultlocale, page)
-    strings = params["localedata"]
-    comments = params["localecomments"]
-    for string_name in strings.iterkeys():
-      strings[string_name] = {"message": strings[string_name]}
-      if string_name in comments:
-        strings[string_name]["description"] = comments[string_name]
-    page_strings[page] = strings
+    cms.utils.process_page(source, defaultlocale, page,
+                           format=format, localized_string_callback=record_string)
   return page_strings
 
-def ensure_required_locales(required_locales, enabled_locales, defaultlocale):

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: configure_locales maybe? This is more about configuring the project than
anything else.

[kzar, 2015/07/02 12:33:11]

Done.

+def configure_locales(crowdin_api, required_locales, enabled_locales,
+                      defaultlocale):
   logger.info("Checking which locales are supported by Crowdin...")
-  response = crowdin_request("GET", "supported-languages")
+  response = crowdin_api.request("GET", "supported-languages")
 
   supported_locales = {l["crowdin_code"] for l in response}
-  skipped_locales = list(required_locales.difference(supported_locales))

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: why convert a set to list here? Just change skipped_locales.append() into
skipped_locales.add() below and it should work.

Nit: skipped_locales = required_locales - supported_locales?

[kzar, 2015/07/02 12:33:12]

Done.

+  skipped_locales = required_locales - supported_locales
 
   if skipped_locales:
-    logger.warning("Ignoring locales that Crowdin doesn't support: %s" % (
-      ", ".join(skipped_locales)
-    ))
-    required_locales = required_locales.intersection(supported_locales)

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: required_locales = required_locales & supported_locales? Or maybe even more
obvious: required_locales -= skipped_locales.

[kzar, 2015/07/02 12:33:12]

Done.

-
-  # It's useful to have a list of all locales to skip
-  skipped_locales.append(defaultlocale)
+    logger.warning("Ignoring locales that Crowdin doesn't support: %s",
+                   ", ".join(skipped_locales))
+    required_locales -= skipped_locales
 
   if not required_locales.issubset(enabled_locales):
     logger.info("Enabling the required locales for the Crowdin project...")
-    crowdin_request(
+    crowdin_api.request(
       "POST", "edit-project",
-      data={"languages[]": list(enabled_locales.union(required_locales))}

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: list(enabled_locales | required_locales) please.

[kzar, 2015/07/02 12:33:11]

Done.

+      data={"languages": enabled_locales | required_locales}
     )
 
-  return required_locales, skipped_locales
+  return required_locales
 
 def list_remote_files(project_info):
   def parse_file_node(node, path=""):
     if node["node_type"] == "file":
       remote_files.add(path + node["name"])
     elif node["node_type"] == "directory":
       dir_name = path + node["name"]
       remote_directories.add(dir_name)
       for file in node.get("files", []):
         parse_file_node(file, dir_name + "/")
 
   remote_files = set()
   remote_directories = set()
-  map(parse_file_node, project_info["files"])

[Wladimir Palant, 2015/06/29 19:05:37]

You are misusing map() here, its callback should not have side-effects - the
result returned should rather be used. I think that here the best option is to
keep it simple:

for node in project_info["files"]:
  parse_file_node(node)

[kzar, 2015/07/02 12:33:12]

Done.

+  for node in project_info["files"]:
+    parse_file_node(node)
   return remote_files, remote_directories
 
 def list_local_files(page_strings):
   local_files = set()
   local_directories = set()
   for page, strings in page_strings.iteritems():
     if strings:
       local_files.add(page + ".json")
-      if "/" in page:
-        parts = page.split("/")[:-1]
-        path = ""
-        while parts:
-          path += parts.pop(0)
-          local_directories.add(path)
-          path += "/"

[Wladimir Palant, 2015/06/29 19:05:36]

I think the following should be simpler:

  local_files.add(page + ".json")
  while "/" in page:
    page = page.rsplit("/", 1)[0]
    local_directories.add(page)

[kzar, 2015/07/02 12:33:10]

Done.

+      while "/" in page:
+        page = page.rsplit("/", 1)[0]
+        local_directories.add(page)
   return local_files, local_directories
 
-def create_directories(directories):
+def create_directories(crowdin_api, directories):
   for directory in directories:
-    logger.info("Creating directory %s" % directory)
-    crowdin_request("POST", "add-directory", data={"name": directory})
-
-def add_update_files(api_endpoint, message, files, page_strings):
-  for group in grouper(files, FILES_PER_REQUEST):
-    files = {}
+    logger.info("Creating directory %s", directory)
+    crowdin_api.request("POST", "add-directory", data={"name": directory})
+
+def add_update_files(crowdin_api, api_endpoint, message, files, page_strings):
+  for group in grouper(files, crowdin_api.FILES_PER_REQUEST):
+    files = []
     for file_name in group:
-      page = file_name[:-5]

[Wladimir Palant, 2015/06/29 19:05:37]

Use os.path.splitext()?

[kzar, 2015/07/02 12:33:12]

Done.

-      files["files[%s]" % file_name] = (file_name, json.dumps(page_strings[page]))
+      page = os.path.splitext(file_name)[0]
+      files.append((file_name, json.dumps(page_strings[page]), "application/json"))
       del page_strings[page]

[Wladimir Palant, 2015/06/29 19:05:37]

Why delete the strings?

[kzar, 2015/07/02 12:33:11]

We no longer need them, and as they might be quite large I thought it was worth
deleting them to avoid wasting memory.

-    logger.info(message % len(files))
-    crowdin_request("POST", api_endpoint, files=files)
-
-def upload_new_files(new_files, page_strings):
-  add_update_files("add-file", "Uploading %d new pages...",
+    logger.info(message, len(files))
+    crowdin_api.request("POST", api_endpoint, files=files)
+
+def upload_new_files(crowdin_api, new_files, page_strings):
+  add_update_files(crowdin_api, "add-file", "Uploading %d new pages...",
                    new_files, page_strings)
 
-def update_existing_files(existing_files, page_strings):
-  add_update_files("update-file", "Updating %d existing pages...",
+def update_existing_files(crowdin_api, existing_files, page_strings):
+  add_update_files(crowdin_api, "update-file", "Updating %d existing pages...",
                    existing_files, page_strings)
 
-def upload_translations(source_dir, new_files, required_locales):
+def upload_translations(crowdin_api, source_dir, new_files, required_locales):
   def open_locale_files(locale, files):
-    for file in files:
-      path = os.path.join(source_dir, "locales", locale, file)
+    for file_name in files:
+      path = os.path.join(source_dir, "locales", locale, file_name)
       if os.path.isfile(path):
-        yield ("files[%s]" % file, open(path, "r"))
+        with open(path, "rb") as f:
+          yield (file_name, f.read(), "application/json")
 
   if new_files:
     for locale in required_locales:
       for files in grouper(open_locale_files(locale, new_files),
-                           FILES_PER_REQUEST):
+                           crowdin_api.FILES_PER_REQUEST):
         logger.info("Uploading %d existing translation "
-                    "files for locale %s..." % (len(files), locale))

[Wladimir Palant, 2015/06/29 19:05:37]

Uploading per locale might result in doing one request for each locale. You
could compile the complete list instead:

  def open_files():
    for locale in required_locales:
      for file in files:
        path = os.path.join(source_dir, "locales", locale, file)
        if os.path.isfile(path):
          yield ("files[%s]" % file, open(path, "r"))

[kzar, 2015/07/02 12:33:12]

This is deliberate, when uploading translations you are allowed to upload
multiple files at once but they must all be for the same language.
https://crowdin.com/page/api/upload-translation

-        crowdin_request("POST", "upload-translation", files=dict(files),
-                        data={"language": locale})
-        for f in files:
-          f[1].close()

[Wladimir Palant, 2015/06/29 19:05:37]

These files should be closed regardless of whether the request fails. Meaning:

  try:
    logger.info(...)
    crowdin_request(...)
  finally:
    for fieldname, file in files:
      file.close()

Note that I'd really prefer deconstructing the tuples properly in the loop
instead of using f[1].

[kzar, 2015/07/02 12:33:10]

Done.

-
-def remove_old_files(old_files):
+                    "files for locale %s...", len(files), locale)
+        crowdin_api.request("POST", "upload-translation", files=files,
+                            data={"language": locale})
+
+def remove_old_files(crowdin_api, old_files):
   for file_name in old_files:
-    logger.info("Removing old file %s" % file_name)
-    crowdin_request("POST", "delete-file", data={"file": file_name})
-
-def remove_old_directories(old_directories):
+    logger.info("Removing old file %s", file_name)
+    crowdin_api.request("POST", "delete-file", data={"file": file_name})
+
+def remove_old_directories(crowdin_api, old_directories):
   for directory in reversed(sorted(old_directories, key=len)):
-    logger.info("Removing old directory %s" % directory)
-    crowdin_request("POST", "delete-directory", data={"name": directory})
-
-def download_translations(source_dir, skipped_locales):
+    logger.info("Removing old directory %s", directory)
+    crowdin_api.request("POST", "delete-directory", data={"name": directory})
+
+def download_translations(crowdin_api, source_dir, required_locales):
   logger.info("Requesting generation of fresh translations archive...")
-  result = crowdin_request("GET", "export")
+  result = crowdin_api.request("GET", "export")
   if result.get("success", {}).get("status") == "skipped":
     logger.warning("Archive generation skipped, either "
                    "no changes or API usage excessive")
 
   logger.info("Downloading translations archive...")
-  response = requests.get(
-    "https://api.crowdin.com/api/project/%s/download/all.zip?key=%s" % (
-      crowdin_project_name, crowdin_api_key
-    )
-  )
-  response.raise_for_status()
+  response = crowdin_api.raw_request("GET", "download/all.zip")
+
   logger.info("Extracting translations archive...")
-  with zipfile.ZipFile(BytesIO(response.content), "r") as archive:
+  with zipfile.ZipFile(io.BytesIO(response.data), "r") as archive:
     locale_path = os.path.join(source_dir, "locales")
     # First clear existing translation files
     for root, dirs, files in os.walk(locale_path, topdown=True):
       if root == locale_path:
-        # Don't delete locale files for unsupported locales or the default
-        dirs[:] = [d for d in dirs if d not in skipped_locales]
+        dirs[:] = [d for d in dirs if d in required_locales]
       for f in files:
-        if f.endswith(".json"):
+        if f.lower().endswith(".json"):
           os.remove(os.path.join(root, f))
     # Then extract the new ones in place
-    archive.extractall(locale_path)

[Wladimir Palant, 2015/06/29 19:05:37]

Running extractall is pretty dangerous IMHO. I'd suggest iterating over all
files, checking that they are valid (not default locale, ending with .json),
then extracting them individually.

[kzar, 2015/07/02 12:33:12]

Done.

+    for member in archive.namelist():
+      path, file_name = posixpath.split(member)
+      ext = posixpath.splitext(file_name)[1]
+      locale = path.split(posixpath.sep)[0]
+      if ext.lower() == ".json" and locale in required_locales:
+        archive.extract(member, locale_path)
 
 def crowdin_sync(source_dir, crowdin_api_key):
-  global crowdin_project_name, defaultlocale
-
   with FileSource(source_dir) as source:
     config = source.read_config()
     defaultlocale = config.get("general", "defaultlocale")
     crowdin_project_name = config.get("general", "crowdin-project-name")
 
+    crowdin_api = CrowdinAPI(crowdin_api_key, crowdin_project_name)
+
     logger.info("Requesting project information...")
-    project_info = crowdin_request("GET", "info")

[Wladimir Palant, 2015/06/29 19:05:37]

Nit: The two lines above and setting enabled_locales can be done outside the
with block. In fact, this seems to belong logically into the
ensure_required_locales function - it should just check which locales are
already configured.

[kzar, 2015/07/02 12:33:11]

I originally didn't request the project information until after extracting the
page strings but during testing I found it to be a real pain. What would often
happen was the API key, project name or something else was wrong and instead of
getting immediate feedback you had to wait several minutes to find out. This way
you know immediately if there's a problem.

+    project_info = crowdin_api.request("GET", "info")
     page_strings = extract_strings(source, defaultlocale)
 
     required_locales = {l for l in source.list_locales() if l != defaultlocale}
     enabled_locales = {l["code"] for l in project_info["languages"]}
 
-  required_locales, skipped_locales = ensure_required_locales(

[Wladimir Palant, 2015/06/29 19:05:37]

Why do we need to know the skipped locales here? The warning has been shown
already, and ignoring them in the download seems to make no sense - the locales
are skipped because they aren't supported by Crowdin, meaning that they cannot
be part of the download.

[kzar, 2015/07/02 12:33:11]

We need to avoid deleting locale files for skipped_locales, otherwise
pre-existing translations for an unsupported locale will be lost. (Perhaps the
locale directory just needs to be renamed so the locale matches what Crowdin
expects?)

[Wladimir Palant, 2015/07/08 23:11:05]

You can (and should) limit deleting to required_locales.

[kzar, 2015/07/11 19:21:15]

Done.

-    required_locales, enabled_locales, defaultlocale
-  )
+  required_locales = configure_locales(crowdin_api, required_locales,
+                                       enabled_locales, defaultlocale)
+
   remote_files, remote_directories = list_remote_files(project_info)
   local_files, local_directories = list_local_files(page_strings)
 
   # Avoid deleting all remote content if there was a problem listing local files
   if not local_files:
-    logger.error("No page strings found. (Wrong project directory?) Aborting!")

[Wladimir Palant, 2015/06/29 19:05:37]

Nit: The settings.ini file is there, so it cannot be a wrong directory. "No
existing strings found, maybe project directory not set up correctly?"

[kzar, 2015/07/02 12:33:11]

Done.

+    logger.error("No existing strings found, maybe the project directory is "
+                 "not set up correctly? Aborting!")
     sys.exit(1)
 
-  new_files = list(local_files.difference(remote_files))
-  new_directories = list(local_directories.difference(remote_directories))

[Wladimir Palant, 2015/06/29 19:05:37]

Nit: I don't see the point converting sets to lists here. Also, can just use the
minus operator.

[kzar, 2015/07/02 12:33:11]

(Cool I found this one myself and later noticed your comment :) )
Done.

-  create_directories(new_directories)
-  upload_new_files(new_files, page_strings)
-  upload_translations(source_dir, new_files, required_locales)
-
-  existing_files = list(local_files.intersection(remote_files))

[Wladimir Palant, 2015/06/29 19:05:36]

Nit: existing_files = local_files - new_files?

[kzar, 2015/07/02 12:33:10]

Done.

-  update_existing_files(existing_files, page_strings)
-
-  old_files = remote_files.difference(local_files)
-  old_directories = remote_directories.difference(local_directories)
-  remove_old_files(old_files)
-  remove_old_directories(old_directories)
-
-  download_translations(source_dir, skipped_locales)
+  new_files = local_files - remote_files
+  new_directories = local_directories - remote_directories
+  create_directories(crowdin_api, new_directories)
+  upload_new_files(crowdin_api, new_files, page_strings)
+  upload_translations(crowdin_api, source_dir, new_files, required_locales)
+
+  existing_files = local_files - new_files
+  update_existing_files(crowdin_api, existing_files, page_strings)
+
+  old_files = remote_files - local_files
+  old_directories = remote_directories - local_directories
+  remove_old_files(crowdin_api, old_files)
+  remove_old_directories(crowdin_api, old_directories)
+
+  download_translations(crowdin_api, source_dir, required_locales)
   logger.info("Crowdin sync completed.")
 
 if __name__ == "__main__":
   if len(sys.argv) < 3:
     print >>sys.stderr, "Usage: python -m cms.bin.translate www_directory crowdin_project_api_key [logging_level]"
     sys.exit(1)
 
   logging.basicConfig()
   logger.setLevel(sys.argv[3] if len(sys.argv) > 3 else logging.INFO)
 
   source_dir, crowdin_api_key = sys.argv[1:3]
   crowdin_sync(source_dir, crowdin_api_key)