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

Better noqa interpretation #717

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Better noqa interpretation #717

merged 1 commit into from
Jan 23, 2025

Conversation

sbrunner
Copy link
Member

@sbrunner sbrunner commented Jan 21, 2025

Description

  • To completely ignore a line the # noqa should be alone
  • To ignore a code we can use # noqa: <code> ad ruff do
  • To ignore a code from a certain source use # noqa: <source>.<code>, this will also create an error if the ignored is not used.

Alternative of #714

Related Issue

Motivation and Context

  • Be compatible with Ruff
  • Have more control on message to be ignored
  • Be notified on used ignore

How Has This Been Tested?

Test added

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Sorry, something went wrong.

@sbrunner sbrunner force-pushed the better-noqa branch 5 times, most recently from e04e7c3 to 65a4641 Compare January 21, 2025 14:17
@sbrunner sbrunner marked this pull request as ready for review January 21, 2025 14:23
@carlio
Copy link
Member

carlio commented Jan 21, 2025

So as I see it, it'll maintain the same behaviour (<-- I use British spelling because I am, but I won't moan about the changes in the docs 😛) for the most part.

Tools with their own syntax will do what they used to do, #noqa is still a blanket "ignore this line".

There is new syntax which is prospector-specific of #noqa flake8.F345 or similar. Prospector I think should be nothing more than a convenience wrapper and shouldn't introduce its own stuff too much, but equally there are time when flake8 or bandit might not have a way to ignore only a single line or only a specific error, so this seems like a good and valid addition to me.

@Pierre-Sassoulas
Copy link
Collaborator

#noqa atool.F345 is not the obvious way atool would implement noqa of a specific message so I would use #noqa: F345 so prospector does not require specific change to be adopted from a codebase where the noqa are already here. There might be collision if errors from various tools are in the same namespace so multiple errors are noqa'ed (flake8 F345 / ruff F345 bandit F345), but I think it's probably okay. In the case of ruff/flake8 it's actually the same message re-implemented, and for bandit/(ruff/flake8) how often is this actually going to happen that there's both a collision in message name between tooling AND one error is mistakenly silenced because this line is rewritten after the noqa was added and those two errors happens on the same line ?

@sbrunner
Copy link
Member Author

@Pierre-Sassoulas what you say is probably true, but I think that having a test that the ignored is really use is an added value, And I can do it only I'm sure that I receive the message and with Ruff it's not the case.

If you still be against that, I will remove it :-)

@Pierre-Sassoulas
Copy link
Collaborator

It's possible to do both Ruff style or '# Noqa: atool: F408' style, so if you're going to use the atool: style it you might as well add it :)

@sbrunner
Copy link
Member Author

@Pierre-Sassoulas Yes It's effectively what I do in this pull request, just with a different syntax,
I propose # noqa: tool.code, you # noqa: tool: code,
but if we have multiple issues, what will it be:
in my case: # noqa: tool1.code1, tool1.code2, tool2.code,
in your case, you imagine: # noqa: tool1: code1, code2; tool2: code?

@Pierre-Sassoulas
Copy link
Collaborator

in your case, you imagine: # noqa: tool1: code1, code2; tool2: code?

I was arguing that ruff style without giving the tool (#noqa: RUF001, F408, with F408 not being a ruff message code, but a code from another tool than ruff) was the one that I would use (in your example that would be # noqa: code1,code2,code).

@sbrunner
Copy link
Member Author

Ok, I will suppress that, but we lost the possibility to be notified on unused ignore :-(

@Pierre-Sassoulas
Copy link
Collaborator

I don't think it's a problem to be able to do both # noqa: code1,code2,code and # noqa: tool1.code1, tool1.code2, tool2.code though (well the added complexity of course) ? Also Carlio's opinion is important and might be blocking, I'm just discussing ideas here.

- To completely ignore a line the `# noqa` should be alone
- To ignore a code we can use `# noqa: <code>` ad ruff do
@sbrunner
Copy link
Member Author

@Pierre-Sassoulas any review?

@sbrunner sbrunner merged commit 2583b91 into master Jan 23, 2025
5 checks passed
@sbrunner sbrunner deleted the better-noqa branch January 23, 2025 10:45
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.

None yet

3 participants