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

Reading from stdin doesn't work with the switch to pycodestyle #1016

Closed
asottile opened this issue Apr 3, 2021 · 9 comments
Closed

Reading from stdin doesn't work with the switch to pycodestyle #1016

asottile opened this issue Apr 3, 2021 · 9 comments

Comments

@asottile
Copy link
Member

asottile commented Apr 3, 2021

In GitLab by @xZise on Jul 13, 2016, 03:02

When the switch from pep8 (2.5) to pycodestyle (2.6) happened, it broke the compatibility of plugins that were using pep8 to get the content. If breaking compatibility was intentional, it should be documented.

This issue is similar to #161 but has a different appearance. And importing pycodestyle instead of pep8 will fix this issue but not #161. Also even though there is a workaround for plugins, this is about either documenting this or providing support for the “old way”.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 13, 2016, 04:01

As in, plugins were unconditionally doing

import pep8

# ....

pep8.get_stdin_value()

or am I misunderstanding this?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @xZise on Jul 13, 2016, 04:21

Yes that is correct.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 13, 2016, 04:45

So do we need to document that if you use pycodestyle instead, it should work as you expect it to? In other words if you do:

import flake8

if flake8.__version__.startswith(('2.6', '3.')):
    import pycodestyle as pep8
else:
    import pep8

Because we still properly patch pycodestyle the way we patched pep8: https://gitlab.com/pycqa/flake8/blob/stable/2.6/flake8/engine.py#L316

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @xZise on Jul 13, 2016, 07:26

I guess that would work. As mentioned in #161 doing from flake8.engine import pep8 does the same thing. Not sure whether either of the two solutions should also be done on Flake8 3.x as there I'll get an empty string when using pycodestyle (but either haven't tested it with pep8 or forgot the result).

And yes the patching works anyways, but it only applies it to the module actually used. And this is afaik why this happens.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 14, 2016, 04:15

And yes the patching works

Right, we can make the patching continue to work in 3.0. That's something I'm aware of and planning to do.

it only applies it to the module actually used

Are you trying to say I should have continued patching pep8 anyway in 2.6? That I should continue patching pep8 as well in 3.0?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @xZise on Jul 14, 2016, 13:57

Well patching in Flake8 3.x is afaik something that should be handled with #161. Regarding patching pep8 in 2.6 (and 3.0): It depends on how you want plugins to handle the problem to get the stdin content. It seems that most plugins chose to use pep8.stdin_get_value, which broke them in 2.6 because you don't patch pep8 anymore but you patch pycodestyle.

Now you could either say that plugin developers have to accomodate for that (and break them) or patch pep8 as well. The former might be a bit harsh (as it was from 2.5 to 2.6, afaik changes shouldn't remove functionality) but the latter is pretty ugly. If your intentions were different and plugins should've done it differently (which didn't break from 2.5 to 2.6) then I don't think patching pep8 should be done in 2.6 as the plugins are using it incorrectly anyways.

Anyway one key part is that at least I'd appreciate if there is some documentation, so that I don't have to test around and figure it out manually, when other plugins probably need to do it as well. And you code snippet with the version check from your comment is the preferred implementation?

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 20, 2016, 06:16

mentioned in merge request !77

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 20, 2016, 06:49

Status changed to closed by commit d69cb05

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @sigmavirus24 on Jul 20, 2016, 12:52

mentioned in commit leorochael/flake8@d69cb0564ada817b1cda5af49a552684466eb2dc

@asottile asottile closed this as completed Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant