Issue 3076 - [CMS] Map locale names to match Crowdin's expectations

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,
@@ skipping 110 matching lines @@
 def configure_locales(crowdin_api, local_locales, enabled_locales,
                       defaultlocale):
   logger.info("Checking which locales are supported by Crowdin...")
   response = crowdin_api.request("GET", "supported-languages")
 
   supported_locales = {l["crowdin_code"] for l in response}
 
   # We need to map the locale names we use to the ones that Crowdin is expecting
   # and at the same time ensure that they are supported.
   required_locales = {}
-  skipped_locales = []
   for locale in local_locales:
     if "_" in locale:

[Sebastian Noack, 2015/09/16 19:37:37]

This can be simplified:

if "_" in locale:
  crowdin_locale = locale.replace("_", "-")
elif locale in supported_locales:
  crowdin_locale = locale
else:
  crowdin_locale = "%s-%s" % (locale, locale.upper())

[kzar, 2015/09/16 20:02:41]

Yea, I did it kinda like that initially but then it bugged me that if locale ==
crowdin_locale we're doing the "in supported_locales" check twice. I'm not sure
which is better, maybe you're right.

[Sebastian Noack, 2015/09/17 09:45:14]

Complexity-wise you have two |in supported_locales| checks either way. But with
the current implementation you are duplicating |required_locales[locale] =| as
well and have an unnecessary loop control instruction, making the code flow
harder to follow. And performance-wise, set lookups have constant time. Not that
performance matters here anyway.

[kzar, 2015/09/17 10:16:16]

Done.

       crowdin_locale = locale.replace("_", "-")
+    elif locale in supported_locales:
+      crowdin_locale = locale
     else:
-      if locale in supported_locales:
-        required_locales[locale] = locale
-        continue
       crowdin_locale = "%s-%s" % (locale, locale.upper())
 
     if crowdin_locale in supported_locales:
       required_locales[locale] = crowdin_locale
     else:
-      skipped_locales.append(locale)
-
-  if skipped_locales:
-    logger.warning("Ignoring locales that Crowdin doesn't support: %s",
-                   ", ".join(skipped_locales))
+      logger.warning("Ignoring locale '%s', which Crowdin doesn't support",
+                     locale)
 
   required_crowdin_locales = set(required_locales.values())
   if not required_crowdin_locales.issubset(enabled_locales):
     logger.info("Enabling the required locales for the Crowdin project...")
     crowdin_api.request(
       "POST", "edit-project",
       data={"languages": enabled_locales | required_crowdin_locales}
     )
 
   return required_locales
@@ skipping 101 matching lines @@
     for member in archive.namelist():
       path, file_name = posixpath.split(member)
       ext = posixpath.splitext(file_name)[1]
       path_parts = path.split(posixpath.sep)
       locale, file_path = path_parts[0], path_parts[1:]
       if ext.lower() == ".json" and locale in inverted_required_locales:
         output_path = os.path.join(
           locale_path, inverted_required_locales[locale],
           *file_path + [file_name]
         )
-        with archive.open(member) as f, open(output_path, "wb") as f_target:

[Sebastian Noack, 2015/09/16 19:37:37]

Why not |archive.extract(member, output_path)|?

[kzar, 2015/09/16 20:02:41]

I believe the path parameter to the extract method is just for the base path,
the path of the file inside the archive and the file name is joined to that. So
if we did what you're suggestion wouldn't the path end up being something like
".../locales/es/some_page.json/es-ES/some_page.json"?

[kzar, 2015/09/16 20:10:29]

(I just tested that to make sure, unfortunately my fears where confirmed :/ )

-          shutil.copyfileobj(f, f_target)
+        with archive.open(member) as source_file, \
+             open(output_path, "wb") as target_file:
+          shutil.copyfileobj(source_file, target_file)
 
 def crowdin_sync(source_dir, crowdin_api_key):
   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_api.request("GET", "info")
     page_strings = extract_strings(source, defaultlocale)
 
-    local_locales = {l for l in source.list_locales() if l != defaultlocale}

[Sebastian Noack, 2015/09/16 19:37:37]

Nit: set(source.list_locales()) - {defaultlocale}

[kzar, 2015/09/16 20:02:41]

Done.

+    local_locales = source.list_locales() - {defaultlocale}
     enabled_locales = {l["code"] for l in project_info["languages"]}
 
   required_locales = configure_locales(crowdin_api, local_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:
@@ skipping 21 matching lines @@
 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)