Noissue - undo accidental logic changes introduced by 78d85c096f9e

cms/converters.py

Index: cms/converters.py
===================================================================
--- a/cms/converters.py
+++ b/cms/converters.py
@@ -8,16 +8,18 @@
 # 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 __future__ import unicode_literals
+
 import os
 import HTMLParser
 import re
 
 import jinja2
 import markdown
 
 
@@ -72,30 +74,30 @@
             self._attrs = None
             self._pagename = None
             self._inside_fixed = False
             self._fixed_strings = None
 
     def handle_starttag(self, tag, attrs):
         if self._inside_fixed:
             raise Exception("Unexpected HTML tag '{}' inside a fixed string"
-                            'on page {}'.format(tag, self._pagename))
+                            ' on page {}'.format(tag, self._pagename))
         if tag == 'fix':

[Wladimir Palant, 2017/03/12 16:02:22]

For reference, I'm not a huge fan of removing every "unnecessary" else
statement. This function exemplifies the point nicely. It handles four mutually
exclusive scenarios, and before the changes you could tell that just by looking
at the overall structure. Now you have to realize that the first branch has a
raise statement in it and so execution won't continue. For the Python
interpreter both versions are identical, but for humans the current version of
the code is harder to read.

From what I can tell, PEP8 doesn't mandate replacing elif by if statement here.
Would also be a rather unpythonic rule, with Python being very much about making
structures obvious.

[Vasily Kuznetsov, 2017/03/13 12:25:29]

I agree that applying strict "no unnecessary else's" rule here makes the logic
less clear. We could of course add returns to other branches and then not have
any elif's or else's at all, but it seems like the original structure was
actually better.

Unfortunately our flake-eyeo plugin is rather strict about this so we seem to
have shot ourselves in the foot a bit. I have created a ticket to deal with this
(https://issues.adblockplus.org/ticket/4980) but for now I will revert the `if`
and add an exception to `tox.ini`.

             self._inside_fixed = True
             self._fixed_strings.append([])
-        if tag in self._whitelist:
+        elif tag in self._whitelist:
             self._attrs.setdefault(tag, []).append(attrs)
             self._string.append('<{}>'.format(tag))
         else:
             raise Exception("Unexpected HTML tag '{}' inside a fixed string"
-                            'on page {}'.format(tag, self._pagename))
+                            ' on page {}'.format(tag, self._pagename))
 
     def handle_endtag(self, tag):
         if tag == 'fix':
-            self._string.append('{{{}}}'.format(self._fixed_strings))
+            self._string.append('{{{}}}'.format(len(self._fixed_strings)))
             self._inside_fixed = False
         else:
             self._string.append('</{}>'.format(tag))
 
     def _append_text(self, s):
         if self._inside_fixed:
             self._fixed_strings[-1].append(s)
         else:
@@ -151,17 +153,17 @@
         # Handle duplicated strings
         if default:
             self._seen_defaults[(page, name)] = (default, comment)
         else:
             try:
                 default, comment = self._seen_defaults[(page, name)]
             except KeyError:
                 raise Exception('Text not yet defined for string {} on page'
-                                '{}'.format(name, page))
+                                ' {}'.format(name, page))
 
         # 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']:
@@ -176,40 +178,40 @@
         # 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}}}'.format(i), fixed_string)
+            result = result.replace('{{{}}}'.format(i), fixed_string)
 
         # Insert attributes
         result = escape(result)
 
         def stringify_attribute((name, value)):
             return '{}="{}"'.format(
                 escape(name),
                 escape(self.insert_localized_strings(value, {}))
             )
 
         for tag in self.whitelist:
-            allowed_contents = '(?:[^<>]|{})'.format('|').join((
+            allowed_contents = '(?:[^<>]|{})'.format('|'.join((
                 '<(?:{}[^<>]*?|/{})>'.format(t, t)
                 for t in map(re.escape, self.whitelist - {tag})
-            ))
+            )))
             saved = saved_attributes.get(tag, [])
             for attrs in saved:
                 attrs = map(stringify_attribute, attrs)
                 result = re.sub(
                     r'{}({}*?){}'.format(re_escape('<{}>'.format(tag)),
                                          allowed_contents,
                                          re_escape('</{}>'.format(tag))),
-                    lambda match: r'<{}{}>{}</{}>'.format(
+                    lambda match: '<{}{}>{}</{}>'.format(

[Wladimir Palant, 2017/03/12 16:02:22]

Like with my other comment, by not using raw strings for regular impressions you
aren't making it any easier for the Python interpreter - yet humans will no
longer see what's going on. More importantly, people changing these regular
expressions will have to remember that they need to switch to raw strings when
adding backslashes.

[Vasily Kuznetsov, 2017/03/13 12:25:29]

I switched from raw strings to unicode strings to address the more strict
behavior of .format with respect to unicode. However, after I added `from
__future__ import unicode_literals` to the top, this is no longer necessary, so
I reverted it to a raw string, which indeed is usually better for regular
expressions.

Thanks for catching this.

                         tag,
                         ' ' + ' '.join(attrs) if attrs else '',
                         match.group(1),
                         tag
                     ),
                     result, 1, flags=re.S
                 )
             result = re.sub(
@@ -281,17 +283,17 @@
 
                     converter = converter_class(self._params,
                                                 key='includedata')
                     result = converter()
                     self.missing_translations += converter.missing_translations
                     self.total_translations += converter.total_translations
                     return result
             raise Exception('Failed to resolve include {}'
-                            'on page {}'.format(name, self._params['page']))
+                            ' on page {}'.format(name, self._params['page']))
 
         return re.sub(
             r'{}\?\s*include\s+([^\s<>"]+)\s*\?{}'.format(
                 self.include_start_regex,
                 self.include_end_regex
             ),
             resolve_include,
             text
@@ -392,17 +394,17 @@
                 path = os.path.join(dirname, filename)
                 namespace = self._params['source'].exec_file(path)
 
                 name = os.path.basename(root)
                 try:
                     dictionary[name] = namespace[name]
                 except KeyError:
                     raise Exception('Expected symbol {} not found'
-                                    'in {}'.format(name, path))
+                                    ' in {}'.format(name, path))
 
         self._env = jinja2.Environment(
             loader=SourceTemplateLoader(self._params['source']),
             autoescape=True)
         self._env.filters.update(filters)
         self._env.globals.update(globals)
 
     def get_html(self, source, filename):