hhatto / autopep8

A tool that automatically formats Python code to conform to the PEP 8 style guide.
https://pypi.org/project/autopep8/
MIT License
4.56k stars 290 forks source link

Don't "fix" errors if the file doesn't parse #544

Open benjaminjkraft opened 4 years ago

benjaminjkraft commented 4 years ago

I have autopep8 set up to run on save in my editor. (And I much appreciate it, thank you!) Sometimes, I save a file that has obvious syntax errors. (Of course I see a bunch of red from my linter, but I ignore those for now, obviously I'm not going to push the code like this.) But autopep8 still tries to run; and often it makes a mess.

In short, I'd like autopep8 to no-op in that case. (Or to have an option to do so.) To see why it's such a problem requires a bit more explanation.

For example, here's some ordinary code:

def do_stuff():
    """Does the stuff; see doers/stuff.py for details."""
    ...

Suppose I start writing a function above this; I write the following and then save.

def prepare_for_stuff():
    """Prepare for a call to do_stuff().

(Note the lack of close-quote.) Python now interprets the def do_stuff as part of the new function's docstring, then the docstring of do_stuff as code. Eventually somewhere it will hit a syntax error (in this case on Does); this code makes no sense as-is.

But the problem is, autopep8 still tries to do what it can, even after the syntax error. In this case, it sees the slash in doers/stuff.py, says "ah, that's a binary operator without whitespace, we can fix that", and changes it to doers / stuff.py.

Inevitably, that code is 2 screens down from where I am, so I don't even notice it. Next, I finish writing the docstring, and write again. But now the doers / stuff.py is back in a docstring, so of course autopep8 doesn't touch it. Eventually, hopefully, I (or -- yikes! -- my code reviewer) notice, and then I get to go through and revert it.

This situation could have been avoided by simply refusing to fix any file that has syntax errors. The syntax errors aren't pep8 compliant either, and once the user fixes those autopep8 can go about its work.

I've tested this on latest autopep8.

benjaminjkraft commented 4 years ago

Looking at the source it seems like only some fixes are applied in this case. So another way to fix this would be to remove the offending fixers to that list. The two I've noticed are:

Others, like E251, sometimes get broken in similar scenarios (usually involving a missing close paren) but autopep8 can fix the induced breakage once the file parses again, so that's less of an issue. But I don't really see how any autofixing whatsoever is likely to be useful to me while the file has a syntax error. Even something simple like E111 (indentation is not a multiple of four) may be intentional in a docstring.

hhatto commented 3 years ago

Would it help if I check the Python syntax on the plugin side and then run autopep8?

for example:

python -m py_compile TARGET.py && autopep8 TARGET.py