Issue 4044 - Added handling for __future__ unicode_literals import to check_quotes()

flake8-abp/flake8_abp.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # 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 360 matching lines @@
 def check_non_default_encoding(physical_line, line_number):
     if line_number <= 2 and re.search(r'^\s*#.*coding[:=]', physical_line):
         return (0, 'A303 non-default file encoding')
 
 check_non_default_encoding.name = 'abp-non-default-encoding'
 check_non_default_encoding.version = __version__
 
 
 def check_quotes(logical_line, tokens, previous_logical, checker_state):
     first_token = True
-    if tokens[0][3] == 1:
-        checker_state['has_unicode_literals'] = False

[Sebastian Noack, 2016/06/01 09:37:52]

Awesome that we can avoid the global variable. However, according to the
documentation, checker_state is a new dict per file. So no need to
initialize/reset the value.

-
-    # check if in unicode_literals mode
+
     token_strings = [t[1] for t in tokens]
     future_import = token_strings[:3] == ['from', '__future__', 'import']
 
     if future_import and 'unicode_literals' in token_strings:
         checker_state['has_unicode_literals'] = True
-
-    has_unicode_literals = checker_state.get('has_unicode_literals')
 
     for kind, token, start, end, _ in tokens:
         if kind == tokenize.INDENT or kind == tokenize.DEDENT:
             continue
 
         if kind == tokenize.STRING:
             match = re.search(r'^([rub]*)([\'"]{1,3})(.*)\2$',
                               token, re.IGNORECASE | re.DOTALL)
             prefixes, quote, text = match.groups()
             prefixes = prefixes.lower()
 
-            if 'u' in prefixes and not has_unicode_literals:

[Vasily Kuznetsov, 2016/06/01 11:01:32]

I wrote it before but maybe it got lost with all these rebases and stuff: if 'u'
prefix is used it's always A112 regardless of `has_unicode_literals`.

[Jon Sonesen, 2016/06/01 17:07:00]

Thanks for the reminder I thought I had removed it but I am mistaken. I will
fixy.

+            if 'u' in prefixes:
                 yield (start, 'A112 use "from __future__ import '
                               'unicode_literals" instead of '
                               'prefixing literals with "u"')
 
             if first_token and re.search(r'^(?:(?:def|class)\s|$)',
                                          previous_logical):
                 if quote != '"""':
                     yield (start, 'A109 use triple double '
                                   'quotes for docstrings')
             elif start[0] != end[0]:

[Vasily Kuznetsov, 2016/06/01 11:01:32]

I have a mild preference for how this looked before with `if start[0] ==
end[0]:` and more branching inside. I think the logic is clearer that way.

[Sebastian Noack, 2016/06/01 11:05:01]

I disagree. That way it's easier to understand that we explicitly ignore
multiline strings (except for docstrings). Also avoiding another level of
indentation, makes it easier to avoid long lines with the now more complex logic
below.

[Vasily Kuznetsov, 2016/06/01 11:34:31]

Yeah, you have a point too. Ok, let's leave it as is.

                 pass
             elif 'r' in prefixes:
                 if quote != "'" and not (quote == '"' and "'" in text):
                     yield (start, 'A110 use single quotes for raw string')
             else:
-                is_unicode = 'u' in prefixes
-                is_bytes = 'b' in prefixes
                 prefix = ''
                 if sys.version_info[0] >= 3:
                     if 'b' in prefixes:
                         prefix = 'b'
-                elif is_unicode or has_unicode_literals and not is_bytes:

[Sebastian Noack, 2016/06/01 09:37:52]

For better code locality please retrieve checker_state['has_unicode_literals']
where it is used, rather than unconditionally above. Also the code is more
consistent, and IMO easier to read, if you avoid the is_unicode and is_bytes
variables.

  if sys.version_info[0] >= 3:
      if 'b' in prefixes:
          prefix = 'b'
  else:
      u_literals = checker_state.get('has_unicode_literals')
      if 'u' in prefixes or u_literals and 'b' not in prefixes:
          prefix = 'u'

[Jon Sonesen, 2016/06/01 17:07:00]

I agree with you regarding the use of the fetch happening where it is used, and
will get that fixed my reasoning for using the variables was that the line was a
bit long but I will gladly change it to the more preferable check without using
variables.

[Jon Sonesen, 2016/06/01 17:14:03]

Ah, prior to reducing the indent level with the change to the elif pass the line
was too long but not anymore!

[Jon Sonesen, 2016/06/02 01:14:10]

Also I just tried to implement the if else logic as you have demonstrated above
and it appeared to introduce a regression where A110 is erroneously thrown.

-                    prefix = 'u'
+                else:
+                    u_literals = checker_state.get('has_unicode_literals')
+                    if 'u' in prefixes or u_literals and 'b' not in prefixes:
+                        prefix = 'u'
 
                 literal = '{0}{1}{2}{1}'.format(prefix, quote, text)
                 if ascii(eval(literal)) != literal:
                     yield (start, "A110 string literal doesn't match "
                                   '{}()'.format(ascii.__name__))
 
         first_token = False
 
 check_quotes.name = 'abp-quotes'
 check_quotes.version = __version__
@@ skipping 44 matching lines @@
                     if tokens[i + 1][:2] != (tokenize.OP, ':'):
                         break
 
                     return [(pos, 'A111 redundant parenthesis for {} '
                                   'statement'.format(statement))]
 
     return []
 
 check_redundant_parenthesis.name = 'abp-redundant-parenthesis'
 check_redundant_parenthesis.version = __version__