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

False positive "E203 whitespace before ':' " on list slice. #373

Open
aleksey-kutepov opened this issue Jan 21, 2015 · 15 comments

Comments

@aleksey-kutepov
Copy link

commented Jan 21, 2015

I've encountered the problem in the following code:

a = [1, 2, 3, 4, 5]
b = a[1+1 : 2+2]  # E203
c = a[1 + 1 : 2 + 2]  # E203
d = a[1+1:2+2]

However, PEP8 chapter https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements handles this as good style:

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. Exception: when a slice parameter is omitted, the space is omitted.

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]
ham[lower+offset : upper+offset]
ham[: upper_fn(x) : step_fn(x)], ham[:: step_fn(x)]
ham[lower + offset : upper + offset]
No:

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

@catgeek86

This comment has been minimized.

Copy link

commented Jan 28, 2015

I took a look at the code and it looks like the function extraneous_whitespace
on line 271 in pep8.py is the function that needs to be updated. It is
returning "E03 whitespace before :" regardless of whether it is a legitimate.

This is confirmed in the doc string for the function, showing that E203 it covers these cases:
E203: if x == 4: print x, y; x, y = y , x
E203: if x == 4: print x, y ; x, y = y, x
E203: if x == 4 : print x, y; x, y = y, x

Looking at the testsuite E20.py confirmed that it is not accounting for legitimate uses of this case.

@catgeek86

This comment has been minimized.

Copy link

commented Jan 28, 2015

This needs to be cleaned up A LOT but here is a prototype of a potential fix(very incomplete but it is a workable idea)
it recognizes ham[lower + offset : upper + offset] as correct and [1 : 2] as incorrect.

Please keep in mind this was a quick very incomplete prototype.
If no one else has a better idea, then I would like to cleanup/finish this idea.

  1. a regex to recognize the colon used as an operator (this needs to be cleaned up and needs operators other than '+' added)
    COLON_AS_OPERATOR = re.compile(r'\S+\s_+\s_\S+\s:\s\S+\s_+\s_\S+')

  2. within the extraneous_whitespace function, it calls a new function to check if the colon is being used as an operator
    if char == ':':
    colon_is_operator = is_colon_operator(logical_line)

  3. The new function is_colon_operator checks to see if it matches the regex as defined
    in COLON_AS_OPERATOR
    def is_colon_operator(logical_line):
    found = 0
    for match in COLON_AS_OPERATOR.finditer(logical_line):
    found = 1
    return found

  4. if the colon is being used as an operator, it does not return an error. Otherwise (as in [1 : 2])
    it returns the 203 error
    if colon_is_operator == False:
    yield found, "%s whitespace before '%s'" % (code, char)

@vodik

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2016

So I've been hacking on this for some time, and I don't know how to approach this without tokenizing. The wall I'm hitting is that the rules are context sensitive.

I've managed to hack the linter enough that it handles expressions inside [] specially. However that breaks when you have lists of dictionaries, where the rules "revert" back inside {}.

I have some code that somewhat works, but I'd be interested in some more guidance on how to tackle this.

@jayvdb

This comment has been minimized.

Copy link

commented Jun 3, 2016

Hi @vodik, you can add a parameter tokens, and if you need state (im not sure it is needed) see use of checker_state in module_imports_on_top_of_file.

@RonnyPfannschmidt

This comment has been minimized.

Copy link

commented May 30, 2018

this is now triggered by black doing the "right" thing - psf/black#279

@sigmavirus24

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@RonnyPfannschmidt I'd be happy to merge a PR that fixes this case

@RonnyPfannschmidt

This comment has been minimized.

Copy link

commented May 30, 2018

@sigmavirus24 i#ll takea short look if i can figure a quickfix

@RonnyPfannschmidt

This comment has been minimized.

Copy link

commented May 30, 2018

no quickfix for this one :(

@webknjaz

This comment has been minimized.

Copy link

commented Jun 20, 2018

Facing the same issue:

-        attr_name = adapter[last_dot + 1:]
+        attr_name = adapter[last_dot + 1 :]
@tommilligan-plutoflume

This comment has been minimized.

Copy link

commented Jun 20, 2018

As a temporary workaround, black will format the following without spacing (passing pycodestyle check):

i = last_dot + 1
attr_name = adapter[i:]
@RonnyPfannschmidt

This comment has been minimized.

Copy link

commented Jun 20, 2018

i simply disabled the check, as black is making those bits correct in all cases

mosbasik added a commit to mosbasik/dotfiles that referenced this issue Oct 23, 2018

peterjc added a commit to peterjc/thapbi-pict that referenced this issue Jan 9, 2019

Configure flake8 to work with black
Using black's default line length of 88.

Black follows the PEP8 standard more closely than
pycodestyle does (as used in the flake8 checks)
so must disable E203 until this is fixed:
PyCQA/pycodestyle#373

peterjc added a commit to peterjc/thapbi-pict that referenced this issue Jan 10, 2019

Configure flake8 to work with black
Using black's default line length of 88.

Black follows the PEP8 standard more closely than
pycodestyle does (as used in the flake8 checks)
so must disable E203 until this is fixed:
PyCQA/pycodestyle#373

jonathanhaigh-arm added a commit to ARMmbed/mbl-tools that referenced this issue Jan 29, 2019

sanity-check.sh: disable some spurious warnings
* The default behaviour of pydocstyle is to ignore "test_" files when
  given a directory to check. Follow its example and ignore "test_"
  files in the invocation we use too.

* Ignore pycodestyle "E203" warnings - sometimes they are spurious
  (PyCQA/pycodestyle#373).

jonathanhaigh-arm added a commit to ARMmbed/mbl-tools that referenced this issue Jan 29, 2019

sanity-check.sh: disable some spurious warnings (#126)
* The default behaviour of pydocstyle is to ignore "test_" files when
  given a directory to check. Follow its example and ignore "test_"
  files in the invocation we use too.

* Ignore pycodestyle "E203" warnings - sometimes they are spurious
  (PyCQA/pycodestyle#373).
@cclauss

This comment has been minimized.

Copy link

commented May 6, 2019

Is there a way to align the projects pycodestyle and black so that users no longer need to manually configure them to work on a common repo? This alignment is now more important given that black has been moved into an official python repo: https://github.com/python/black

@peterjc

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@cclauss did you mean aside from fixing this issue (#373 in pycodestyle)?

Slightly tangental, but I wrote flake8-black to let me run the black style checks as well as the pycodestyle checks from within flake8, see:

https://github.com/peterjc/flake8-black

I had to document ignoring E203 as a workaround.

@asottile

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Is there a way to align the projects pycodestyle and black so that users no longer need to manually configure them to work on a common repo? This alignment is now more important given that black has been moved into an official python repo: https://github.com/python/black

fwiw the move into the python project is because it is a PSF project now

From this:

Q: Does it mean it's official now?
A: No, just like mypy isn't.

@adithyabsk

This comment has been minimized.

Copy link

commented Jul 15, 2019

Thought I would bump this issue to see where it stood on the priority list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.