Skip to content

Be able to configure message suppression#714

Closed
sbrunner wants to merge 1 commit intomasterfrom
configure-noqa
Closed

Be able to configure message suppression#714
sbrunner wants to merge 1 commit intomasterfrom
configure-noqa

Conversation

@sbrunner
Copy link
Copy Markdown
Member

Description

Be able to configure the suppression, add # prospector: ignore=<source>.<code>

Motivation and Context

I don't like the # noqa because he is too generic, => be able to deactivate it

Add a new suppression to be able to suppress only one error code

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)

@sbrunner sbrunner force-pushed the configure-noqa branch 4 times, most recently from 85f0f5c to 043571c Compare January 20, 2025 13:46
@sbrunner sbrunner marked this pull request as ready for review January 20, 2025 13:49
Copy link
Copy Markdown
Collaborator

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I feel like this is adding yet another standard on top of the existing pylint: disable and noqa from Ruff/flake8 and type:ignore from mypy. Not sure if it's a good thing. (In pylint there's an issue to make noqa work on top of pylint: disable to avoid having various standard). Once you use this it makes the codebase prospector dependant.

@sbrunner sbrunner force-pushed the configure-noqa branch 3 times, most recently from 20f9a75 to 672e714 Compare January 20, 2025 15:23
@sbrunner
Copy link
Copy Markdown
Member Author

I just removed the adding of prospector: ignore=...
Note that the current prospector implementation of noqa interferes with ruff because with Ruff, it's possible to ignore a list of code, and the prospector implementation will ignore all the errors!
https://docs.astral.sh/ruff/linter/#error-suppression

@sbrunner
Copy link
Copy Markdown
Member Author

And I create in issue for the rest of the code: #715 :-)

Copy link
Copy Markdown
Collaborator

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Why introduce 'flake8: noqa' when flake8 itself uses noqa: and Ruff that include flake8 also use raw noqa ? It force the user to know from which software a message is coming to noqa it which make the tool harder to use without a lot of prior knowledge. Flake8 isn't asking the user to know that it's a collection of 3 tools for exemple. It would be annoying to have to do '# pycodestyle: noqa' or '# pyflake: noqa' or '# McCabe: noqa'. (And if you use a number of tool you'll end up with devilish line like '# type ignore noqa: ruf001 flake8: noqa: F408')

@sbrunner
Copy link
Copy Markdown
Member Author

With the current code, I didn't introduce anything except the possibility to disable the prospector # noqa and the #pyflake: noqa that can have bad interaction with, e.g. Ruff.
For example, I add # noqa: F841 to disable only one Ruff rule, and it disables all the rules including Bandit.

In my code, I don't like to ignore all the issues with won comment because with that, we don't know what's ignored,
we don't know if new code introduces a new issue on the same line.

Finally, we can prefer to have the control with a long line or just ignore anything that we don't know with a short line.
It's a chose and I don't know that Prospector should do this chose for you :-)

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator

For example, I add # noqa: F841 to disable only one Ruff rule, and it disables all the rules including Bandit.

This is pretty bad, but shouldn't it be fixed in bandit ?

@sbrunner
Copy link
Copy Markdown
Member Author

Fix in bandit that Prospector hide his issue?

With the current code at least we have # noqa on a line of the code Prospector will filter all the issues on this line, it's a Prospector issue not a Bandit one!

@carlio
Copy link
Copy Markdown
Member

carlio commented Jan 21, 2025

I'm not quite following here, so please correct me.

Is the problem that disabling a message for ruff will disable the equivalent message from other tools? If so that's intended behaviour of the "blender" which tries to remove duplicate warnings. This can be disabled with the --no-blending flag or config.

Or is the problem that #noqa: F123 is incorrectly being applied to all messages from other tools, rather than a specific one? So while you might have pylint: disable=F123 the #noqa: F123 is being incorrectly used by prospector as a generic #noqa? In which case it sounds like better parsing/processing of prospector is necessary more than new features.

Or what am I missing?

@sbrunner
Copy link
Copy Markdown
Member Author

Hello @carlio it's the second, I can work on a better implementation or # noqa... :-)

@Pierre-Sassoulas
Copy link
Copy Markdown
Collaborator

My bad I misunderstood what you were saying.

@sbrunner
Copy link
Copy Markdown
Member Author

No problème @Pierre-Sassoulas :-)

@sbrunner sbrunner mentioned this pull request Jan 21, 2025
3 tasks
@sbrunner sbrunner closed this Jan 23, 2025
@sbrunner sbrunner deleted the configure-noqa branch January 23, 2025 11:27
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.

3 participants