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

E301 regression vs 2.5.0 #908

Closed
asottile opened this issue Feb 17, 2020 · 12 comments · Fixed by #927
Closed

E301 regression vs 2.5.0 #908

asottile opened this issue Feb 17, 2020 · 12 comments · Fixed by #927

Comments

@asottile
Copy link
Member

sample

# ok in both
class C(Protocol):
    def f(self) -> int: ...
    def g(self) -> int: ...

# only not ok on master
class D(Protocol):
    @property
    def f(self) -> int: ...
    @property
    def g(self) -> int: ...

before

$ ~/opt/venv/bin/pycodestyle --version
2.5.0
$ ~/opt/venv/bin/pycodestyle t.py
$

after

$ git -C venv/src/pycodestyle/ rev-parse HEAD
d69c15eb7ecf77e94988fb55207a78936b48079c
$ venv/bin/pycodestyle t.py
t.py:11:5: E301 expected 1 blank line, found 0

bisect

$ git bisect start
$ git bisect bad
$ git bisect good 2.5.0
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[7493e0a0e76f02640b6ff0f7cb1619747101a1c6] Fix E302 false negative in presence of decorators.
$ git bisect run python3 -m pycodestyle ../t.py
running python3 -m pycodestyle ../t.py
../t.py:11:5: E301 expected 1 blank line, found 0
Bisecting: 6 revisions left to test after this (roughly 3 steps)
[d1e29861597f22d01e0f528aaccdb744d5bade81] Merge pull request #847 from adamchainz/E225_boolean_operators
running python3 -m pycodestyle ../t.py
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[b8db33dfa6cd4029b75efd1e7cab7c1b64d03bc7] Fixes some issues with E741 detection
running python3 -m pycodestyle ../t.py
Bisecting: 1 revision left to test after this (roughly 1 step)
[3cfd527929fc5dfcdc13a0edf6a6d13f5eb76fcb] Add line breaks in comment
running python3 -m pycodestyle ../t.py
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[96d2db0fa17cc41ae45bcc6fa5ca72e6f712d1bf] Merge pull request #853 from rettenbs/master
running python3 -m pycodestyle ../t.py
7493e0a0e76f02640b6ff0f7cb1619747101a1c6 is the first bad commit
commit 7493e0a0e76f02640b6ff0f7cb1619747101a1c6
Author: Antony Lee <anntzer.lee@gmail.com>
Date:   Tue Mar 26 18:01:22 2019 +0100

    Fix E302 false negative in presence of decorators.

:100755 100755 03696b7a04d5071d75676bdd2a1eefc22559a0a5 ec6b894d117c796e55cfe35724cfd6d7e60eb661 M	pycodestyle.py
:040000 040000 e796744e186d875888aa2143a20b97ccd57e8172 643b9730bb7ed077aec41c43eecb0123f6f6c0ae M	testsuite
bisect run success

claims 7493e0a (CC @anntzer) is the regressing commit

@asottile
Copy link
Member Author

this is a simpler case that also demonstrates this:

def f(): pass

# c
@h
def g(): pass

@anntzer
Copy link
Contributor

anntzer commented Feb 17, 2020

I'm confused; why would you want these to be allowable?
Back in 2.4.0 these would raise an E302/E301. In #823 I additionally allowed one-liners to not have surrounding blank lines, but once you have a decorator it's not a one-liner anymore, as pointed out in #858, so in #859 I put back the error when the decorators are present, effectively reverting to the 2.4.0 behavior, which I think is correct.

@asottile
Copy link
Member Author

#858/#859 was about non-oneliners, I guess the question becomes "does adding a decorator to a one-liner make it no longer a one-liner" (my leaning is that they are still one-liners when decorated?)

@asottile
Copy link
Member Author

(my motivation here is mostly for .pyi stubs and inline Protocol definitions)

@anntzer
Copy link
Contributor

anntzer commented Feb 17, 2020

I would argue they are not oneliners anymore (literally they are not...), so at least personally I think the 2.4.0 behavior is fine.

@sigmavirus24
Copy link
Member

I'm not worried about .pyi stubs as to the last of my recollection flake8/pycodestyle barely work correctly on those files if at all and neither default to linting them.

Protocol definitions as far as I can tell (because these are new to me) are asyncio related, is that right? I've not seen this in practice and would love a more concrete example to wrap my head around. I can empathize with both perspectives but don't have enough understanding to have a firm opinion.

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2020

(I'm not dead set against changing the behavior, I just think the current one is correct. I'll thus unsubscribe from the issue, but feel free to ping me again if needed.)

@asottile
Copy link
Member Author

Protocols are used for "structural subtyping" (mypy) -- they allow you to describe the interface you expect and use that as a type without needing to change the inheritance hierarchy of a type. As such, they're basically stub classes. PEP 544 specifies how they work and provides some examples.

Protocols also can currently be used to work around circular types in mypy (which understands and supports circular Protocols but not circular concrete types).

Here's a few examples where I'm utilizing a protocols:

  • defining a MutableSequenceNoSlice type to provide a base class between list and my custom ListSpy type (roughly mirroring MutableSequence but with a more strict __getitem__ which forbids slicing)
  • using class _Rule(Protocol): to allow Rule to be circular

@sigmavirus24
Copy link
Member

Hm, I'm inclined to agree with @asottile here. @asottile I'd merge a PR that makes this work better since it sounds like Antony doesn't have interest in making changes to this logic.

@asottile
Copy link
Member Author

sounds good -- I'll try and work on something that makes this work better (and shore up the tests around here as well, since I think there's a lot of "undefined space" around E30*)

@asottile
Copy link
Member Author

here's another typing-related case I'd like to try and fix as well:

@overload
def environment_dir(d: None, language_version: str) -> None: ...
@overload
def environment_dir(d: str, language_version: str) -> str: ...

@asottile
Copy link
Member Author

I've added a patch for this in #927

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.

3 participants