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

Fix false alarm in E203 rule (issue #373, black incompatibility) #914

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DesbyP
Copy link

@DesbyP DesbyP commented Feb 28, 2020

Issue #373

Rule E203 checks for extraneous whitespace before a colon.
Yet there is a corner case when the colon is used as a "slice" operator. PEP8 says:

However, in a slice the colon acts like a binary operator, and should have equal amounts on either side (treating it as the operator with the lowest priority). In an extended slice, both colons must have the same amount of spacing applied.

This behaviour isn't implemented and leads to conflict between pycodestyle and black.

Solution

This PR adds to pycodestyle the tolerance for the slice situation, without enforcing the "same amount of space on both sides of the colon" rule.
This way the current Python that code was pycodestyle compliant stays compliant.

Performance

On a Python code base of over 15k lines, a test with hyperfine shows no performance loss.

E203 checks for extraneous whitespace before a colon.
Yet the colon can be used as a "slice" operator, and as such PEP8 asks for 0 or 1 whitespaces on both sides of the operator.
@asottile
Copy link
Member

does this handle: x[lambda : foo]

@DesbyP
Copy link
Author

DesbyP commented Feb 28, 2020

@asottile good point.
I just added support for this case + tests.

@asottile
Copy link
Member

does this handle: [lambda x={tuple([1]):2}: x]

@DesbyP
Copy link
Author

DesbyP commented Feb 28, 2020

@asottile yup

@asottile
Copy link
Member

?

$ git rev-parse HEAD
c88d21ecce0dd0a9d77b7bd2c88024809f8b6f13
$ python3 -m pycodestyle t.py
$ echo $?
0

I expect the inner : to be an error

@DesbyP
Copy link
Author

DesbyP commented Mar 2, 2020

the PR modifies only the portion of code that treats extra whitespaces, it shouldn't change current behaviour about missing whitespaces.

This seems to be a different issue in pycodestyle, indeed the current version on branch master doesn't detect anything in this case either :

$ git checkout master
$ python3 -m pycodestyle t.py
$ echo $?
0

@DesbyP
Copy link
Author

DesbyP commented Mar 16, 2020

Hi
is it ok to merge ?

@DesbyP
Copy link
Author

DesbyP commented Mar 27, 2020

@asottile is it ok for you ?

@asottile
Copy link
Member

sorry I haven't had a chance to look at the updated code

@DesbyP
Copy link
Author

DesbyP commented Apr 23, 2020

Hi @asottile, I hope you're ok in these troubled times.
It would be great if you could take a look at the code, this issue is a blocker for flake8 and black adoption in my company.
Thanks

@asottile
Copy link
Member

I don't see how this is a blocker, you can use the suggested flake8 configuration from black

this change is quite complicated (adds a lot of edges and magic numbers) and I have not had time to look at every fine detail of it

@DesbyP
Copy link
Author

DesbyP commented Apr 23, 2020

I understand that this is not trivial to deploy.

We have a lot of projects dispersed in many teams, I can't promote a seamless integration of black if I ask to tweak flake8 configuration on every one of them.

@asottile
Copy link
Member

We have a lot of projects dispersed in many teams, I can't promote a seamless integration of black if I ask to tweak flake8 configuration on every one of them.

why not? you'll have to modify all of the code in the repository and add some configuration for black already

@DesbyP
Copy link
Author

DesbyP commented Apr 23, 2020

not necessarily, black can be used out of the box without config
the goal is to integrate it as a background formatter in PyCharm

@pranasziaukas
Copy link

pranasziaukas commented Jun 4, 2020

In my opinion, the fact of how the current E203 rule deals with slices is an unintended consequence.
Even worse, it makes PEP8-compliant code raise warnings, so it's not just about black.

So I agree that the fix is needed.

Edit: to illustrate, our pipeline fails at

for combo in combos[i : i + batch_size]:

which is very obviously PEP8-compliant, even listed as a correct usage example here.

@kislyuk
Copy link

kislyuk commented Jul 7, 2020

It seems there is consensus that E203 is not a PEP8 style compliance error and should be disabled by default. The presence of the rule for E203 in the default configuration prevents flake8 and black, among other tools, from interoperating smoothly. Could the maintainer please consider merging this PR?

Copy link

@hoylemd hoylemd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I had one very minor suggestion re: formatting of multi-line conditions, but it's not a big deal at all.

I'm also not quite sure it's for me to approve the PR, so I'm commenting instead of Approving so I don't step on anyone's toes.

Comment on lines +477 to +478
elif (line[found - 1] != ',' and
not (char == ':' and is_a_slice(line, found))):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif (line[found - 1] != ',' and
not (char == ':' and is_a_slice(line, found))):
elif (line[found - 1] != ','
and not (char == ':' and is_a_slice(line, found))):

I was under the impression that 'pythonic' style puts the binary operator after a line break, not before.

--- Personal preference / opinion only below this line. Disregard at your pleasure ---

I personally also prefer to format multi-line conditions like this:

        elif (
            line[found - 1] != ','
            and not (char == ':' and is_a_slice(line, found))
        ):

because it puts co-ordinate conditions in a visually co-ordinate structure, and also has dedicated lines to mark the bounds of the compound condition. (i.e. the elif ( and ): lines, visually positioned to encapsulate the conditions and indicate that they're subordinate to the elif construct itself. It does take up a lot more vertical space though, so I can understand why some might dislike that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoylemd I agree and prefer this coding style as well.
Yet W503 in TravisCI prevents me of doing so :

pycodestyle.py:489:13: W503 line break before binary operator
pycodestyle.py:512:13: W503 line break before binary operator

If @s-weigand suggestion to ignore W503 was accepted then we could proceed :)

Copy link
Contributor

@FichteFoll FichteFoll Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that 'pythonic' style puts the binary operator after a line break, not before.

This was changed only a few years back (roughly when W504 was introduced and both were added to the default ignore list). pycodestyle itself does not use this style of line breaks before binary operators and you should always adhere to the project's style.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the precision
Then I guess the PR is ok for review

@s-weigand
Copy link

s-weigand commented Jul 31, 2020

Since W503 (which lets the travis tests fail) was added to ignore default:

* Added W503 to the list of codes ignored by default ignore list; #498

IMHO it should also be added to the tests config
ignore = E226,E24,W504

Or maybe just use the default ignore.

s-weigand added a commit to s-weigand/pyglotaran that referenced this pull request Aug 31, 2020
This is a temporary fix until the issue is resolved, see PyCQA/pycodestyle#914
s-weigand added a commit to s-weigand/pyglotaran that referenced this pull request Sep 1, 2020
This is a temporary fix until the issue is resolved, see PyCQA/pycodestyle#914
kdeldycke added a commit to kdeldycke/mail-deduplicate that referenced this pull request Dec 7, 2020
kdeldycke added a commit to kdeldycke/mail-deduplicate that referenced this pull request Dec 7, 2020
@marctorsoc
Copy link

What's the status of this?

johnsca added a commit to juju-solutions/loadbalancer-interface that referenced this pull request Feb 10, 2021
johnsca added a commit to juju-solutions/loadbalancer-interface that referenced this pull request Feb 10, 2021
* Add Python Black format check and apply format changes

Bring this repo in line with our standards to use Python Black
formatting conventions, and enforce it in the lint checks.

* Split lint from tests and only run on 3.9 (black requires 3.6+)

* Ignore spurious E203 from flake8 (see PyCQA/pycodestyle#914)
@DesbyP
Copy link
Author

DesbyP commented Feb 16, 2021

Hi @asottile,
The activity on this PR shows that a lot of people are waiting for this fix and are forced to use the workaround. Would you consider reviewing my commits ?
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 this pull request may close these issues.

8 participants