New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3 fixes - fix contrib/python checkstyle #6274

Merged
merged 7 commits into from Jul 31, 2018

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jul 30, 2018

Beyond the usual unicode vs bytes issues, the AST grammar changed substantially between Python 2 vs 3. For example, ast.TryExcept was renamed to ast.Try. See http://joao.npimentel.net/2015/07/23/python-2-vs-python-3-ast-differences/ for all the changes.

@@ -91,15 +91,15 @@ def _remove_coding_header(cls, blob):
"""
# Remove the # coding=utf-8 to avoid AST erroneous parse errors
# https://bugs.python.org/issue22221
lines = blob.split('\n')
lines = blob.decode('utf-8').split('\n')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

I kept blob always being binary string—even though I don't think it's necessary to read these source files as binary—to stay closer to original semantics.

lines are always unicode now. Earlier it was ambiguous what they were.

@@ -273,7 +274,7 @@ def __init__(self, code, severity, filename, message, line_range=None, lines=Non
def __str__(self):
"""convert ascii for safe terminal output"""
flat = list(self.flatten_lines([self.message], self.lines))
return '\n |'.join(flat).encode('ascii', errors='replace')
return '\n |'.join(flat).encode('ascii', errors='replace').decode('ascii')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

__str__ must return unicode in Py3 no matter what.

This function will still strip out any non-ascii, though, and then spit it back out as unicode.

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

That's probably comment worthy.

if PY3:
# Python 3 interpreter will raise SyntaxError upon reading a print statement.
# So, this module cannot be meaningfully used when ran with a Python 3 interpreter.
return

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

I'm not sure what to do about this. There is no way for Python 3 to parse a print statement, even if it's reading Python 2.

To reproduce, open Python 3 console:

import ast
ast.dump(ast.parse('print "hi"'))

I'm concerned this implies once we switch Pants to Python 3 under-the-hood, it will start to complain about Python 2 only syntax like print statements and except Error, e syntax? If so, this means users still on Python 2 will have to convert all Py2-only syntax to Py2-3 compliant to use the newest version of Pants. I see no way around this, and think it is fairly acceptable because they do not have to fully migrate to Python 3.

Still, we should communicate well with users about what must be changed and how to use the futurize script to do it easily.

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

cc @CMLivingston has had some thoughts about this, which @jsirois recently incorporated: see how #6182 dynamically creates a pex to run a linter.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Interesting. So before we release a stable version of Pants running Python 3, we could create a Pex to run this contrib code using Py2 interpreter?

There's a counterpoint, the Python 2 interpreter will complain about Python 3 only syntax, like type hints

# Python2
ast.dump(ast.parse('x: int = 4'))

So, I don't think there's a way we could support linting for both Python 2-only syntax and Python 3-only syntax? We could get fancy and use the compatibility flag to trigger the corresponding linter for that module, perhaps. The common subset of Python 2 and Python 3 of course will work as well, like running Python 3 linter over Pants in its current Python 2 state.

It seems to me that unless we had two PEXes and dynamically chose which to use depending on the file, we'll have to choose between supporting Py 2 only + Py 2/3 subset, or Py3 only + Py 2/3 subset.

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

Dynamically creating the linter pex means you don't need to create "N" static pexes for N python versions... just linter code that is compatible with all of the (major) python versions you want to lint. You'd then dynamically create a pex for python 3 if you needed to lint 3, etc.

'except KeyError, e :',
'except (KeyError, ValueError), e\t:'):
self.assertNit(EXCEPT_TEMPLATE.format(clause), 'T601')
except CheckSyntaxError: # Fix Python 3 raising SyntaxError

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

See above discussion about print statements raising syntax error. Feeding the above input into assertNit raises a SyntaxError.

self.assertItemsEqual(['T403', 'T405', 'T402'],
[chunk_error.code for chunk_error in chunk_errors])
self.assertItemsEqual([ImportType.STDLIB, ImportType.THIRD_PARTY], module_types)
self.assertEqual(sorted(['T403', 'T405', 'T402']),

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

assertItemsEqual was renamed in Python3 to assertCountEqual. Calling sorted has the same effect, and is more explicit and cleaner than choosing the test function according to interpreter version.

Refer to https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertItemsEqual

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -273,7 +274,7 @@ def __init__(self, code, severity, filename, message, line_range=None, lines=Non
def __str__(self):
"""convert ascii for safe terminal output"""
flat = list(self.flatten_lines([self.message], self.lines))
return '\n |'.join(flat).encode('ascii', errors='replace')
return '\n |'.join(flat).encode('ascii', errors='replace').decode('ascii')

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

That's probably comment worthy.

if PY3:
# Python 3 interpreter will raise SyntaxError upon reading a print statement.
# So, this module cannot be meaningfully used when ran with a Python 3 interpreter.
return

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

cc @CMLivingston has had some thoughts about this, which @jsirois recently incorporated: see how #6182 dynamically creates a pex to run a linter.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 30, 2018

It looks like there is 1 substantive travis failure.

@stuhood stuhood merged commit 4a4e481 into pantsbuild:master Jul 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_contrib-python branch Aug 11, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Python 3 fixes - fix contrib/python checkstyle (pantsbuild#6274)
Beyond the usual unicode vs bytes issues, the AST grammar changed substantially between Python 2 vs 3. For example, `ast.TryExcept` was renamed to `ast.Try`. See http://joao.npimentel.net/2015/07/23/python-2-vs-python-3-ast-differences/ for all the changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment