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.

Sign up for GitHub

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

Jump to bottom

E241 not supported with noqa comment #996

Closed
fmigneault opened this issue May 13, 2021 · 9 comments · May be fixed by #997
Closed

E241 not supported with noqa comment #996

fmigneault opened this issue May 13, 2021 · 9 comments · May be fixed by #997

Comments

@fmigneault
Copy link

The # noqa: E241 comment notation is not respected by pycodestyles.
This is extremely inconvenient for following reasons:

  1. It is not obvious at first glance that pycodestyle discriminates between some codes and not others when using # noqa (as described in intro). This causes harder interpretation of # noqa cross the code.

  2. pycodestyle allows and already correctly handles ignore = E241 via the config or corresponding input argument when applied globally. So on one side it advertises some form of control of ignored/selected errors, but on the other enforces for no specific reason to not support # noqa: E241 locally (although some other # noqa codes work).

  3. The doc seems to somewhat contradict itself by not allowing # noqa specifically for E241. It is said that E241 is among codes that are ignored by default:

    (*) In the default configuration, the checks E121, E123, E126, E133, E226, E241, E242, E704, W503, W504 and W505 are ignored because they are not rules unanimously accepted, and PEP 8 does not enforce them.

    But then, if I need to provide more/other codes to ignore than the defaults (--ignore or config ignore = ), I loose the flexibility to control which lines they apply to, although it is agreed that PEP8 don't enforce them.

I am aware that issue #924 was closed saying that flake8 can be used instead to respect that notation, and this is "fine", as I use it also that way and it works. The problem arises when trying "fix" those issues.

I am using tool autopep8, which under the covers uses pycodestyle to apply fixes.
flake8 does not provide fixing feature, so I have to go with autopep8 which respects what pycodestyle tells it.

I also do not want to apply ignore = E241 globally, because I want to catch cases where incorrect indents and spaces where added by mistake. I want only E241 to be ignored for explicit cases where indentation provides more readability (e.g.: in tests with many aligned columns of corresponding values), where E241 should be disabled explicitly and only for those lines. This also follows PEP8 mentality that one should be allowed to bypass recommendations especially for cases of improving readability.

For all these reasons, I would like to request that pycodestyle increases the number of error codes it supports via # noqa and let the user customize local overrides, but with my main concern around E241 specifically if all codes makes the change scope too big.
@asottile
Copy link
Member

pycodestyle does not interpret # noqa: E123 -- it only sees # noqa -- the code-specific handling is a feature of flake8

@fmigneault
Copy link
Author

@asottile
I have tried also replacing my # noqa: E241 with generic # noqa, and still I get the same result that they are reported.
So # noqa are not respected either.

pycodestyle --select E241 --max-line-length=120 .

image

fmigneault added a commit to crim-ca/weaver that referenced this issue May 13, 2021
@fmigneault
hack around error codes for "fix-lint" makefile target because "pycod…
34f80fd 
…estyle" doesn't respect them via "noqa" notation (see PyCQA/pycodestyle#996)
fmigneault added a commit to crim-ca/weaver that referenced this issue May 13, 2021
@fmigneault
hack around error codes for "fix-lint" makefile target because "pycod…
feda525 
…estyle" doesn't respect them via "noqa" notation (see PyCQA/pycodestyle#996)
@asottile
Copy link
Member

there are a few codes in pycodestyle which (intentionally) do not support noqa from pycodestyle's side -- E241 appears to be one of them (the reasoning predates me so I don't know)

fmigneault added a commit to crim-ca/weaver that referenced this issue May 13, 2021
@fmigneault
reapply hack around pycodestyle not respecting noqa E241 via autopep8…
cf9be78 
… (see PyCQA/pycodestyle#996)
@fmigneault fmigneault mentioned this issue May 13, 2021
Merged
@fmigneault
Copy link
Author

Yes. I am aware that support is intentionally removed in that case. But why?
The reason why I request to review this decision is that it makes no sense to me.

Many "visual indentation" related codes are supported (E121-E131 marked by (^) in https://github.com/PyCQA/pycodestyle/blob/master/docs/intro.rst), but some such as E241 are not for no real reason, although they apply for similar concepts related to indentation.

I believe pycodestyle shouldn't ignore some errors. I should either do all or none, especially since it can already ignore them if the setting is set globally.

@fmigneault fmigneault mentioned this issue May 13, 2021
Open
@fmigneault
Copy link
Author

@asottile
See above proposed PR.
I believe the change is as simple as adding the noqa parameter, demonstrating that there really shouldn't be any reason to not allow these kind of overrides.

It ran for py27, py37 and py38 by tox on my machine and pass,
If not sufficient, I will be happy to look into improving further the PR.

@sigmavirus24
Copy link
Member

I'm fairly certain some of the reasoning is in the issue tracker as this wasn't arbitrarily pulled out of thin air. I would guess that part of the reasoning was that all E24* codes are ignored by default and if someone wants to turn that on, they probably don't want to disable it and all other errors on that line, if they do, why are they enabling it?

As to the PR, it's tiny and doesn't harm anything, and at the same time (if we had telemetry, I could be certain) I would guess fewer than 1% of our users know about or use E24* codes for checking meaning that fixing this is almost without any impact on users. Further, there are better ways to be enabling error codes that you just want to ignore - Flake8.

@fmigneault
Copy link
Author

@sigmavirus24
I agree with you that for checking only, flake8 will be more powerful and specific about the code.
The problem I have is really related about the fixing feature that autopep8 provides via pycodestyle reporting, so I cannot use flake8 directly.
I don't want to disable E24* codes altogether because it is only a few places to display matrices of values that purposely don't respect them for readability. I still want to flag bad code linting in other places to preserve good code quality.

@asottile
Copy link
Member

sounds like you can get what you want by disabling that rule in autopep8?

@fmigneault
Copy link
Author

@asottile I can disable it globally, but not locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add support of E241 ignore by noqa comment inline fmigneault/pycodestyle
3 participants
@sigmavirus24 @asottile @fmigneault