Issue 2625 - [cms] Crowdin synchronisation script

cms/converters.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 122 matching lines @@
     def re_escape(s):
       return re.escape(escape(s))
 
     # Extract tag attributes from default string
     default, saved_attributes, fixed_strings = self._attribute_parser.parse(default, self._params["page"])
 
     # Get translation
     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:
       result = default
       self.missing_translations += 1
     self.total_translations += 1
+
+    # Perform callback with the string if required, e.g. for the translations script
+    callback = self._params["localized_string_callback"]
+    if callback:
+      callback(page, locale, name, result, comment, fixed_strings)
+
 
     # Insert fixed strings
     for i, fixed_string in enumerate(fixed_strings, 1):
       result = result.replace("{%d}" % i, fixed_string)
 
     # Insert attributes
     result = escape(result)
     def stringify_attribute((name, value)):
       return '%s="%s"' % (
         escape(name),
@@ skipping 240 matching lines @@
         stack.pop()
       stack[-1]["subitems"].append(item)
       stack.append(item)
     return structured
 
 converters = {
   "html": RawConverter,
   "md": MarkdownConverter,
   "tmpl": TemplateConverter,
 }