Noissue - undo accidental logic changes introduced by 78d85c096f9e

cms/converters.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 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
@@ skipping 62 matching lines @@
             self._string = None
             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))
-        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`.

+        elif tag == 'fix':
             self._inside_fixed = True
             self._fixed_strings.append([])
         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))
 
     def handle_endtag(self, tag):
@@ skipping 105 matching lines @@
                 '<(?:{}[^<>]*?|/{})>'.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: '<{}{}>{}</{}>'.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.

+                    lambda match: r'<{}{}>{}</{}>'.format(
                         tag,
                         ' ' + ' '.join(attrs) if attrs else '',
                         match.group(1),
                         tag
                     ),
                     result, 1, flags=re.S
                 )
             result = re.sub(
                 r'{}({}*?){}'.format(re_escape('<{}>'.format(tag)),
                                      allowed_contents,
@@ skipping 259 matching lines @@
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }