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

Slicing is not formatted according to PEP 8 #157

Closed
ambv opened this issue Apr 21, 2018 · 6 comments
Closed

Slicing is not formatted according to PEP 8 #157

ambv opened this issue Apr 21, 2018 · 6 comments
Assignees
Labels
help wanted Extra attention is needed T: enhancement New feature or request T: style What do we want Blackened code to look like?

Comments

@ambv
Copy link
Collaborator

ambv commented Apr 21, 2018

I always found what other formatters are doing here wrong and currently Black is also doing the wrong thing.

Needs changing

The explicit PEP 8 recommendation is:

YES:

ham[lower + offset : upper + offset]
ham[: upper_fn(x) : step_fn(x)], ham[:: step_fn(x)]

NO:

ham[lower + offset:upper + offset]
ham[:upper_fn(x):step_fn(x)], ham[::step_fn(x)]

Essentially, we should treat : as an "operator" inside slices when any operand of the slice includes punctuation (dots, brackets, and other math, binary or logic operators).

More examples:
YES

ham[lower * unit + offset * unit : upper * unit + offset * unit]

NO:

ham[lower*unit+offset*unit : upper*unit+offset*unit]

Note that PEP 8 allows for the following that we cannot properly enforce:

ham[lower+offset : upper+offset]

Black won't do that because "lower", "upper", or "offset" can be arbitrary complexity and then operator hugging isn't improving readability anymore.

Already well formatted

Black is already formatting the other PEP 8 recommendation well:

YES:

ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:]
ham[lower:upper], ham[lower:upper:], ham[lower::step]

NO:

ham[1: 9], ham[1 :9], ham[1:9 :3]
ham[lower : : upper]
ham[ : upper]

(edited: a previous version of this issue suggested hugging operators but I since realized this cannot be performed consistently)

@ambv ambv added T: enhancement New feature or request help wanted Extra attention is needed T: style What do we want Blackened code to look like? labels Apr 21, 2018
@zsol zsol self-assigned this Apr 29, 2018
zsol added a commit that referenced this issue Apr 29, 2018
zsol added a commit that referenced this issue Apr 29, 2018
@zsol
Copy link
Collaborator

zsol commented Apr 29, 2018

I opened a pull request that tries addressing this. I made a couple more decisions along the way. I think these should be considered well-formatted:

slice[::-1]
slice[:c, c - 1]
slice[c, c + 1, d::] 
slice[ham[c::d] :: 1]
slice[ham[cheese ** 2 : -1] : 1 : 1, ham[1:2]]

@ambv ambv closed this as completed in #178 May 1, 2018
ambv pushed a commit that referenced this issue May 1, 2018
@zvezdan
Copy link

zvezdan commented Aug 25, 2018

@ambv PEP-8 says this:

ham[lower+offset : upper+offset]

not what you quoted above:

ham[lower + offset : upper + offset]

Notice that the original from PEP-8 does not have whitespace around the + operator.

The parenthesized reference to "as the operator with the lowest priority" in PEP-8 Pet Peeves section is related to the following exception on whitespace around operators in PEP-8 that few people pay attention to.

YES:

x = x*2 - 1

NO:

x = x * 2 - 1

Normally, according to PEP-8, we would write x * 2 with the spaces around the operator, but, in this case, to make precedence rules more readable, the higher priority operator -- multiplication -- was cuddled with the operands (x and 2) and whitespace was left around the lower priority operator only -- a subtraction.

Similarly, what PEP-8 is saying about the slicing operator is that it is the "lowest precedence" and therefore it has the spaces around it, but notice that also the higher precedence addition (+) has no spaces around it. That is why it should be written as ham[lower+offset : upper+offset] and not the way it seems to be interpreted by Black.

Similarly, in the Black's README the example given as ham[1 + 1 :] should be really ham[1+1 :]. Even better, I'd change the example to something more similar to PEP-8 where this makes more sense than with a simple 1+1 because we can always write that as 2. Also, the documentation would benefit from the clarification that Black will not change ham[1:5] into ham[1 : 5] because that would be non-compliant with PEP-8 too.

@ambv
Copy link
Collaborator Author

ambv commented Aug 26, 2018

Choosing whether to hug math operators or split them with spaces is not easy so Black will not do it. See #148 for a reference why not.

PEP-8 says this:

ham[lower+offset : upper+offset]

not what you quoted above:

ham[lower + offset : upper + offset]

That's not true. The example you're claiming is missing is literally taken from PEP 8. Both variants are allowed in PEP 8.

@zvezdan
Copy link

zvezdan commented Aug 26, 2018

@ambv To paraphrase the color quote: "We can have it both ways, as long as it's Black's way." :-)
Fair enough. It's definitely in the spirit of Black.

Thanks for pointing out #148. When seen together with this issue, it may raise some questions.

According to the answers in these two issues, one exception is easy to implement (space around slice operator), and the other is hard to implement (no space around highest priority math operators). Still, both are exceptions to standard rules: "no space around simple slice operator", such as [1:5] and "space around math operators", such as 1 + 2.

The fact is that, in the math operator case, Black chooses to (over)simplify by always using the standard rule, at the cost of making the original math expressions unreadable. In the slice operator case, it chooses to implement the exception, because it's easy, at the benefit of very small readability improvement. I doubt people have too many issues with reading cuddled slice operators the way pycodestyle simplifies them. Simplifying both cases seems a reasonable choice too.

By implementing this slice operator exception, Black gets a little higher percentage of PEP-8 compliance, albeit, at the price of making the choice between the two equally acceptable slice operator exceptions, as you point out. The first formatting of that exception, as I pointed out, satisfies both the math and the slice operator exceptions, providing even more of PEP-8 compliance. The reason why one exception formatting, among the two, was not chosen by Black, reveals, through issue #148, how hard it is to implement all these exceptions.

Unfortunately, because of that choice, Black changes ham[lower+offset : upper+offset] that is already PEP-8 compliant (mathematicians might argue that it's even more compliant) into the other formatting choice, for no good reason.

Thus, this kind of cherry-picking the PEP-8 exceptions may feel arbitrary and subjective.
That, in turn, can make Black less acceptable to lots of teams and developers.

Personally, I accept the fact that Black developers already made their decisions and that this issue is closed. Thanks!

@muppetjones
Copy link

Just to add a relevant point to save other people the trouble:

  • black does violate pep8 here, but
  • pycodestyle incorrectly tags the pep8 recommendation as E203.

The real issue: Whitespace around colons in slices is broken in pycodestyle.
See (373 here)[https://github.com/PyCQA/pycodestyle/issues/373]

There are a couple other black issues related to this topic:

A simpler and more thorough example:

a[b-1:b+1]  # no complaint
a[b-1 :b+1]
a[b-1: b+1]  # no complaint
a[b-1 : b+1]  # recommended by pep8
a[b - 1:b + 1]  # no complaint
a[b - 1 : b + 1]

a[b-1:]  # no complaint
a[b-1 :]
a[b - 1:]  # no complaint
a[b - 1 :]
a[b - 1 : ]
  • black consistently formats the above as a[b - 1 : b + 1] and a[b - 1 :]
  • pycodestyle (and thus flake8) will complain about most of the above lines with E203 whitespace before ':'
  • pycodestyle complains about the recommended slicing format.

While I'd ordinarily argue that black should not violate pep8, pycodestyle can't even figure it out.

@JelleZijlstra
Copy link
Collaborator

What makes you say that Black is violating "pep8" (not sure if by that you mean the PEP or some tool that purports to enfroce it)?

R1j1t added a commit to R1j1t/contextualSpellCheck that referenced this issue Oct 10, 2020
add E203 ignore to flake8
Ref: psf/black#157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed T: enhancement New feature or request T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

5 participants