NoIssue - Improves converters.py PEP8 compliance

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 58 matching lines @@
                     self._attrs, [''.join(s) for s in self._fixed_strings])
         finally:
             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'

[Vasily Kuznetsov, 2017/03/08 17:31:44]

Do you think it's ok that we're changing the quotes in the string? Why not just
do "bla bla bla '{}' bla bla".format(foo, bar)?

[Jon Sonesen, 2017/03/08 17:51:57]

Well, the otherway causes a style warning, however this may actually be a bug in
flake8-abp in which case I should change it back and file an issue for
flake8-abp.

[Jon Sonesen, 2017/03/08 17:55:25]

whoops, I was wrong plz disregard XD

[Vasily Kuznetsov, 2017/03/09 12:33:27]

Yeah, the logic is that if the string has single quotes in it, the easiest way
is to just use double quotes around it. Basically it's always possible to check
what the "right" way is by doing repr(your_string) in Python 2 or
ascii(your_string) in Python 3.

+            raise Exception("Unexpected HTML tag '{}' inside a fixed string"
                             'on page {}'.format(tag, self._pagename))
         if tag == 'fix':
             self._inside_fixed = True
             self._fixed_strings.append([])
         if 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'
+            raise Exception("Unexpected HTML tag '{}' inside a fixed string"
                             'on page {}'.format(tag, self._pagename))
 
     def handle_endtag(self, tag):
         if tag == 'fix':
             self._string.append('{{{}}}'.format(self._fixed_strings))
             self._inside_fixed = False
         else:
             self._string.append('</{}>'.format(tag))
 
     def _append_text(self, s):
@@ skipping 34 matching lines @@
                 break
             name, value = line.split('=', 1)
             params[name.strip()] = value.strip()
             lines[i] = '\n'
         params[key] = (''.join(lines), filename)
 
     def localize_string(
             self, page, name, default, comment, localedata, escapes):
 
         def escape(s):
             return re.sub(r'.',

[Vasily Kuznetsov, 2017/03/08 17:31:44]

This is also kind of hard to read but more importantly, this is some serious
case of excessive regexp love! Matching r'.', seriously? We'd need to rewrite
this, but not in this ticket, so it's ok for now :)

[Jon Sonesen, 2017/03/08 17:51:57]

I agree here, but assumed it would require an issue ticket.

[Vasily Kuznetsov, 2017/03/09 12:33:27]

Yeah, you're right.

                           lambda match: escapes.get(match.group(0),
                                                     match.group(0)),
                           s, flags=re.S)
 
         def re_escape(s):
             return re.escape(escape(s))
 
         # 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))
 
         # Extract tag attributes from default string
-        default, saved_attributes, fixed_strings = (self._attribute_parser

[Vasily Kuznetsov, 2017/03/08 17:31:44]

This looks kind of hard to read. How about this instead?

        default, saved_attributes, fixed_strings = (
            self._attribute_parser.parse(default, self._params['page'])
        )

[Jon Sonesen, 2017/03/08 17:51:57]

Yes that seems nicer, thank you!

-                                                    .parse(default,
-                                                           self._params['page']
-                                                           ))
+        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
         elif name in localedata:
             result = localedata[name].strip()
         else:
             result = default
             self.missing_translations += 1
@@ skipping 69 matching lines @@
             lookup_string,
             text,
             flags=re.S
         )
 
     def process_links(self, text):
         def process_link(match):
             pre, attr, url, post = match.groups()
             url = jinja2.Markup(url).unescape()
 
-            locale, new_url = self._params['source']\

[Vasily Kuznetsov, 2017/03/08 17:31:44]

PEP8 discourages line continuation with backslashes. Perhaps we could do
something like this:

            locale, new_url = self._params['source'].resolve_link(
                url, self._params['locale']
            )

or even this:

            source = self._params['source']
            locale, new_url = source.resolve_link(url, self._params['locale'])

what do you think?

[Jon Sonesen, 2017/03/08 17:51:57]

oh yeah, I though i replaced all the back slashes I had added..whoops!

-                .resolve_link(url, self._params['locale'])
+            locale, new_url = (
+                self._params['source']
+                .resolve_link(url, self._params['locale']))
+
             if new_url is not None:
                 url = new_url
                 if attr == 'href':
                     post += ' hreflang="{}"'\

[Vasily Kuznetsov, 2017/03/08 17:31:44]

Maybe we can use continuation with parentheses instead?

                    post += (' hreflang="{}"'
                             .format(jinja2.Markup.escape(locale)))

Same below.

[Jon Sonesen, 2017/03/08 17:51:57]

yesh

                         .format(jinja2.Markup.escape(locale))
 
             return ''.join((pre, jinja2.Markup.escape(url), post))
 
         text = re.sub(r'(<a\s[^<>]*\b(href)=\")([^<>\"]+)(\")',
                       process_link, text)
         text = re.sub(r'(<img\s[^<>]*\b(src)=\")([^<>\"]+)(\")',
                       process_link, text)
         return text
 
     include_start_regex = '<'
     include_end_regex = '>'
 
     def resolve_includes(self, text):
         def resolve_include(match):
             name = match.group(1)
             for format_, converter_class in converters.iteritems():
                 if self._params['source'].has_include(name, format_):
-                    self._params['includedata'] = self._params['source']\
-                        .read_include(name, format)
+                    self._params['includedata'] = (
+                        self._params['source'].read_include(name, format))
+
                     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']))
 
         return re.sub(
@@ skipping 185 matching lines @@
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }