Issue 2625 - [cms] Crowdin synchronisation script

cms/converters.py

Index: cms/converters.py
diff --git a/cms/converters.py b/cms/converters.py
index 8f602f3f36149a3b70ac17f6054197a84756b056..94c06f03b964bce26122824440056bc2f9edfa19 100644
--- a/cms/converters.py
+++ b/cms/converters.py
@@ -125,7 +125,7 @@ class Converter:
       params[name.strip()] = value.strip()
     params[key] = "".join(lines)
 
-  def localize_string(self, name, default, localedata, escapes):
+  def localize_string(self, page, name, default, comment, localedata, escapes):
     def escape(s):
       return re.sub(r".",
         lambda match: escapes.get(match.group(0), match.group(0)),
@@ -140,6 +140,10 @@ class Converter:
     locale = self._params["locale"]
     if locale == self._params["defaultlocale"]:
       result = default
+      # Store the default string if required, e.g. for the translations script
+      if self._params["record_default_strings"]:

[Sebastian Noack, 2015/07/08 13:03:20]

Is there any particular reason we call that function only for the default
locale? This shouldn't be necessary as we only generate the default locale when
we extract translations, right?

Also note that record_default_settings will also be available as variable in
jinja templates, if you add it to the _params dictionary. So maybe it would be
better and simpler to patch Converter.localize_string in cms.bin.translate
instead passing that callback around? Note that this would be in line with how
cms.bin.generate_static_files is implemented.

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

That's assumptions, the kind that might not be true tomorrow.
Patch a method to memoize return value without any assumptions about parameters
is one thing, wrapping such a complex function something entirely different -
that would move many assumptions about its functionality into a completely
unrelated piece of code. If anything, we could set the callback as an attribute
on Converter *instances* from cms.utils.get_page_params - "inheriting" this
callback would introduce significant complexity however and doesn't seem to be
worth it.

[Sebastian Noack, 2015/07/09 21:26:55]

Or the other way around. As far as I see it this code shouldn't be in the
default locale code path and the name shouldn't imply it. This will make the
logic of that "param" less complicated to understand.
 
Fair enough.

[Wladimir Palant, 2015/07/10 21:24:04]

True. If we generalize the parameter name into something like
"localized_string_callback" then we can just as well record all strings and let
the callback choose which ones it needs. It should be called with |result| as
parameter rather than |default| of course.

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

Done.

+        self._params["record_default_strings"](page, name, default,
+                                               comment, fixed_strings)
     elif name in localedata:
       result = localedata[name].strip()
     else:
@@ -185,9 +189,8 @@ class Converter:
       name, comment, default = match.groups()
       default = to_html(default).strip()
 
-      # Note: We currently ignore the comment, it is only relevant when
-      # generating the master translation.
-      return self.localize_string(name, default, self._params["localedata"], escapes)
+      return self.localize_string(self._params["page"], name, default,
+                                  comment, self._params["localedata"], escapes)
 
     return re.sub(
       r"{{\s*"
@@ -352,15 +355,17 @@ class TemplateConverter(Converter):
     return result
 
   def translate(self, default, name, comment=None):
-    # Note: We currently ignore the comment, it is only relevant when
-    # generating the master translation.
-    localedata = self._params["localedata"]
-    return jinja2.Markup(self.localize_string(name, default, localedata, html_escapes))
+    return jinja2.Markup(self.localize_string(
+      self._params["page"], name, default, comment,
+      self._params["localedata"], html_escapes
+    ))
 
   def get_string(self, name, page):
     localedata = self._params["source"].read_locale(self._params["locale"], page)
     default = localedata[name]
-    return jinja2.Markup(self.localize_string(name, default, localedata, html_escapes))
+    return jinja2.Markup(self.localize_string(
+      page, name, default, "", localedata, html_escapes
+    ))
 
   def get_page_content(self, page, locale=None):
     from cms.utils import get_page_params