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

Current implementation of --diff is kind of broken by design #1389

Closed
asottile opened this issue Sep 16, 2021 · 1 comment · Fixed by #1441
Closed

Current implementation of --diff is kind of broken by design #1389

asottile opened this issue Sep 16, 2021 · 1 comment · Fixed by #1441

Comments

@asottile
Copy link
Member

--diff currently makes assumptions that code changes will introduce lint errors at the position of the code change which may have been true for pycodestyle which only concerns itself with whitespace / style issues -- but it is not true for pyflakes or generally true for flake8 plugins

here is a small concrete example:

 import os
 
 def main():
-    print(os.getcwd())
+    print('hi')

flake8 t.py reports F401 on line 1, but flake8 --diff silently passes (even though the diff caused that new issue)

as such, I think the option's behaviour needs to be changed in an incompatible way, or removed entirely

if kept, I think the only reasonable behaviour would be to limit changes to the file which a diff touched -- rather than the lines which were touched

@sigmavirus24
Copy link
Member

--diff currently makes assumptions that code changes will introduce lint errors at the position of the code change which may have been true for pycodestyle which only concerns itself with whitespace / style issues -- but it is not true for pyflakes or generally true for flake8 plugins

Yeah this sounds right. 3.x reimplemented the features that 1.x and 2.x relied on which we used by importing and monkey-patching stuff in pep8 directly. Even with it being a backwards incompatible release, I tried to keep as much backwards compatibility as possible.

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

Successfully merging a pull request may close this issue.

2 participants