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 26 matching lines @@
     're.match': 're.search',
     'codecs.open': 'io.open',
 }
 
 ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply', 'buffer', 'coerce',
                                            'intern', 'file'}
 
 LEAVE_BLOCK = (ast.Return, ast.Raise, ast.Continue, ast.Break)
 VOLATILE = object()
 
+IS_UNICODE_LITERALS = False

[Sebastian Noack, 2016/05/25 08:31:36]

Uppercase notation is only for variables that are considered constant (i.e.
never change).

+
 
 def evaluate(node):
     try:
         return eval(compile(ast.Expression(node), '', 'eval'), {})
     except Exception:
         return VOLATILE
 
 
 def is_const(node):
     return evaluate(node) is not VOLATILE
@@ skipping 313 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):
     first_token = True
-    global is_unicode_literals
+    global IS_UNICODE_LITERALS
+
+    # --- check if this is beginning of file

[Sebastian Noack, 2016/05/25 08:31:36]

We generally don use --- in comments. So please remove it for consistency.

[Vasily Kuznetsov, 2016/05/25 13:55:31]

Also pep8 recommends capitalising the first letter in a sentence (see
https://www.python.org/dev/peps/pep-0008/#comments).

+    if tokens[0][3][0] == 1:
+        IS_UNICODE_LITERALS = False
+
+    # --- check if in unicode_literals mode
+    token_strings = [t[1] for t in tokens]

[Sebastian Noack, 2016/05/25 08:31:35]

I wonder whether we should also check for the token type to be NAME.

[Vasily Kuznetsov, 2016/05/25 13:55:32]

Do we really need to? Can you imagine a line that starts with ['from',
'__future__', 'import'] and has 'unicode_literals' in the tail and they are not
names?

[Sebastian Noack, 2016/05/25 14:45:29]

I feel that checking for the token type is more correct (less code assumptions).
But I guess, I don't insist, if it makes the check so much simpler.

[Vasily Kuznetsov, 2016/05/25 16:25:56]

I see, it does feel a bit sloppy to just check the content of the tokens and not
the type. But after more thinking about it, it seems reasonable to me in this
case. What we're assuming is some facts about syntax of Python and it seems like
a pretty safe assumption. So I'd probably prefer keeping the check as it is,
since it's shorter and easier to read this way.

+    if token_strings[:3] == ['from', '__future__', 'import']:
+        IS_UNICODE_LITERALS = 'unicode_literals' in token_strings

[Vasily Kuznetsov, 2016/05/25 13:55:32]

Won't this break if it gets a piece of code like this?

from __future__ import unicode_literals
from __future__ import print_function

We probably only need to turn on the unicode literals flag when we see that
'unicode_literals' is imported and we never want to turn it off (save for first
line of the file).

 
     for kind, token, start, end, _ in tokens:
         if kind == tokenize.INDENT or kind == tokenize.DEDENT:
             continue
 
-        if start[0] == 1:
-            is_unicode_literals = False
-
-        if logical_line == 'from __future__ import unicode_literals':
-            is_unicode_literals = True
-
         if kind == tokenize.STRING:
             match = re.search(r'^(u)?(b)?(r)?((""")?.*)$',
                               token, re.IGNORECASE | re.DOTALL)
             (is_unicode, is_bytes, is_raw,
              literal, has_doc_quotes) = match.groups()
 
             if first_token and re.search(r'^(?:(?:def|class)\s|$)',
                                          previous_logical):
                 if not has_doc_quotes:
                     yield (start, 'A109 use triple double '
                                   'quotes for docstrings')
                 elif is_unicode or is_bytes or is_raw:
                     yield (start, "A109 don't use u'', b'' "
                                   "or r'' for doc strings")
             elif start[0] == end[0]:
                 if is_raw:
                     literal = re.sub(r'\\(?!{})'.format(literal[0]),
                                      '\\\\\\\\', literal)
-
                 if sys.version_info[0] >= 3:
                     if is_bytes:
                         literal = 'b' + literal
-                elif is_unicode or is_unicode_literals:
+                elif is_unicode and not IS_UNICODE_LITERALS:

[Sebastian Noack, 2016/05/25 08:31:35]

It seems the check for IS_UNICODE_LITERALS is incorrect. Also the check for
is_unicode should probably go even above the check for Python 3. After all, the
message indicates, and we agreed on, to never use strings like u''.

[Vasily Kuznetsov, 2016/05/25 13:55:32]

I second that it should come before the Python 3 check. Also, even if we yield
A112, we should still yield the other errors, so if we see something like
u"foo", we get both A110 and A112.

[Sebastian Noack, 2016/05/25 14:45:29]

Just an idea, how about moving it even above the check for docstrings and merge
A112 with A109? Docstrings using r or b is probably a case we can give up on. I
never saw them in real code, and since Python 3 strings prefixed with b are not
even recognized as docstrings.

[Vasily Kuznetsov, 2016/05/25 16:25:56]

Moving the check to above the check for docstrings seems like a good idea.

As for merging, I suppose you mean replacing lines 408-410 with this new check
and raising A112 in that case. This sounds good too.

+                    yield(start, 'A112 use "from __future__ import"'

[Sebastian Noack, 2016/05/25 08:31:35]

There should be a space after "yield". Otherwise it looks like yield is a
function that we call. However, it's a statement and a tuple.

[Sebastian Noack, 2016/05/25 08:31:35]

Please document A112 in the README.

+                          'unicode_literals instead of prefixing'

[Sebastian Noack, 2016/05/25 08:31:35]

Please indent long error messages like it's done in the rest of this script,
i.e. align the starting quotes.

Also note that you forgot to add spaces in the end. So this message will be
printed as:

  A112 use "from __future__ import"unicode_literals instead of prefixingliterals
with "u"

+                          'literals with "u"')
+                elif not is_bytes:
                     literal = 'u' + literal
 
                 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__