Skip to content
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

Refactoring Python 2.7 files does not work (parse error on print statement) #119

Closed
anentropic opened this issue Feb 29, 2020 · 4 comments · Fixed by #123
Closed

Refactoring Python 2.7 files does not work (parse error on print statement) #119

anentropic opened this issue Feb 29, 2020 · 4 comments · Fixed by #123

Comments

@anentropic
Copy link
Contributor

anentropic commented Feb 29, 2020

https://pybowler.io/docs/basics-intro

lib2to3 provides a concrete syntax tree (CST) implementation that recognizes and supports the grammar of all Python versions back to 2.6.

By building on lib2to3, Bowler is capable of reading and modifying source files written for both Python 2 and 3. That said, Bowler requires Python 3.6 or newer to run

Everything was working fine until I tried running on a (Py 2.7) source file which had a print statement in it

Then I get:

ERROR:RefactoringTool:Can't parse tests/fixtures/py27.py: ParseError: bad input: type=3, value='"Python 2 syntax"', context=(' ', (34, 10))

After putting some breakpoints in and digging through the source it seems the problem is here:

options["print_function"] = True

BowlerTool hard-codes it so that the underlying fissix RefactorTool will use the no-print-statements grammar, which is therefore not Python 2 compatible.

Commenting out that line fixes it for me... I wonder why it was added?

@anentropic
Copy link
Contributor Author

anentropic commented Feb 29, 2020

Oh right, I have seen this earlier issue #60

Firstly the docs change seems insufficient, it shouldn't advertise Python 2 support anywhere without mentioning this important caveat.

Secondly, if this is the reason:

when you allow print statements, print functions with keyword arguments fail, unless there is a from __future__ import print_function statement, which obviously isn't required for >3 code

...it seems like it would be better to make this an option at the user level and just explain the problem - maybe I know all my py2 files use print statements rather than "parenthesised print statements without an explicit future import" in which case I'm fine to run with the other grammar

@anentropic
Copy link
Contributor Author

anentropic commented Feb 29, 2020

I mean... the behaviour without this forced option seems correct to me

if I comment it out I can parse:

if __name__ == "__main__":
    print "Python 2 print statement"
    print("parenthesised print statement")

If I add a print function with kwarg:

if __name__ == "__main__":
    print "Python 2 print statement"
    print("parenthesised print statement")
    print("print function with kwarg", end="")

it fails with parse error on the last line (but it should right? this is invalid python 2 - if I run the file in an interpreter I get a SyntaxError from this line)

then if I add the future import:

from __future__ import print_function

def main():
    print "Python 2 print statement"
    print("parenthesised print statement")
    print("print function with kwarg", end="")

I get parse error from the print statement instead... But I should, because a python 2.7 interpreter gives a SyntaxError on this line.

Finally removing the print statement and keeping the future import, no errors as expected.

So I don't understand what you're trying to work around, it seems like fissix already does the right thing here?

Is it that Python 3 files are handled wrongly? I guess they're auto-detected as python 2 because they lack the future import, and then print functions with kwarg will error.

But in that case it seems again like the right thing to do would be expose it as a user option - I am unlikely to be refactoring a mix of py2 and p3 files in the same run. And as long as we don't mix files of different versions I can set the option I want (forced print function grammar for py3, auto-detect for py2) and everything will work correctly.

@thatch
Copy link
Contributor

thatch commented Feb 29, 2020

I think the intent was to parse print(foo) as a function call, not print statement followed by grouping parens, because internally we already ran the 2to3 fixer for print functions before bowler.

I would accept a pull request that adds such an option to Query.

@anentropic
Copy link
Contributor Author

great I'll send one, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants