-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[feature request] Support ignore block of code with noqa #3711
Comments
I'd really like to support something like this. But we may want to couple it with a more holistic redesign of suppression comments. \cc @MichaReiser |
And thank you for the kind words :) |
Hello, just to add to the already wonderful suggestion, I think also adding a simple and and off switch with just |
What's your expectation if a file only contains an |
That the program ignores the lines under that comment. A markdown linter named markdownlint has that option and if I am not mistaken, if there is no This program could also have an option to warn the user when an off flag/comment is unterminated (has no corresponding on switch) cuz disabling linting for the rest of the file is not recommended???? I dont knowwwwwwww |
I'm pretty interested in this one. Obviously pretty new to the project, but I think that the design proposed above makes sense. @MichaReiser saw you were mentioned above, are we good to move forward on this design? Do we want to support ignoring multiple rules at once? |
Technically: I recommend waiting till after #3931 lands because it significantly changes how we handle noqa comments internally. Regarding the design. I haven't had much time to look into suppression comments, I got sidetracked by figuring out how a setting format could look like in a world where ruff does formatting and linting (and more?). My personal objective for suppression comments is that we should try to keep it simple. I really like Rust's approach where there's a single way to enable or disable lints: Using I'm not proposing that we need to implement the same semantics but I think its worthwhile to list the use cases with examples on how the new suppression comments should work. Do we support whole file suppression? Do we want to warn about missing |
That makes sense. I'll start working on a design/use-cases - since #1054 asks for block/file-scoped |
Editing out the design approach I had here to link to the discussion instead: #4051 |
FYI - duplicate of #1289 |
@woile Thanks for opening this issue! Per discussion in #3868 , would you please add the following checklist to the beginning of this issue for PR tracking:
There are several feature requests to add lint ignoring/suppression capabilities, each with slight variations, and some of the project maintainers have asked to keep track of them here. They may need to be implemented at different times or in several stages, so a checklist would be helpful for tracking to PRs. Thank you! |
FWIW, I believe suppression for any scope as in Without knowing the internals, I imagine the former would require reparsing the AST, and thus a significant rewrite. However,
Rather than rewrite the source for every kind of scope feature request, this would be done in one change and would likely cover 99% of use cases for most users. |
I'd like to emphasize the importance of this feature for ruff's usability. There are some situations where you need more sophisticated rule control, for example this: def get_all(cursor, table_name):
return cursor.execute(
'''
SELECT *
FROM {table_name};
'''.format(table_name=table_name)
) ruff rightfully complains that this code contains a possible sql injection (S608). We know that the value of |
Why don't you pass it as a param like you should? Sorry, but this is a perfectly true positive, even if in THIS particular case it's safe - until someone decides to make |
Passing it as a param works in this simple case, but when you build a whole query programmatically …? |
Sorry, my example was chosen very poorly, I've replaced it with a less frivolous one where you can't get away with parameter-passing. |
@flbraun I believe your example might be worth raising a separate issue. I would have expected that one of those work, but none of them do. def get_all(cursor, table_name):
return cursor.execute(
f'''
SELECT *
FROM {table_name};
''' # noqa: S608
)
def get_all(cursor, table_name):
return cursor.execute(
(
"""
SELECT *
FROM {table_name};
""" # noqa: S608
).format(table_name=table_name)
) |
Is there any ongoing work on this issue? |
We are also waiting for this feature Our use case is mainly in tests, we like to disable whitespaces rules the time of an assertions, to present a list/tuple/dict as a table, with aligned rows. self.assertEquals(
# pylint: disable=bad-whitespace
report,
[
['Header', 400.00, 1600.00, 100.00],
['Other header', 00.00, 600.00, 10000.00],
['Some Header', 140.00, 0.00, 1000.00],
['', 1400.00, 600.00, 100.00],
['Finally', 10400.00, 600.00, 100.00],
]
) |
Maybe instead of # ruff: off
...
# ruff: on
# ruff: off: T201
print()
if ...:
print("...", a, b, c)
...
# ruff: on And since we have already |
I think there's a performance aspect to consider. In pylint the fine grained message control sometime prevented us from bypassing costly processing because checking if a message is disabled or not (on a directory, file, scope, line, node, or token) is costly in itself. |
I stumbled upon another use case where this would be useful: A line of code causes one error for mypy and one for bandit. Both support inline-comments for ignoring it, but they don't support ignoring code blocks. I can only ignore the error for one of them and have to ignore the whole file for another (not nice). If I used Ruff (with this feature) as a replacement for Bandit, I could ignore the block of code (1 line) and also add the inline ignore comment for mypy. |
Why not just put two inline comments on the same line? |
Just to chime in with my $0.02, I also think a good linter should have the ability to ignore a block
It should ignore the rest of the file (but turn itself back on at the end of the file) |
Another use case for this. I wanted to leave a comment explaining why I'm turning another rule off, which includes a snippet of a minimal example for a pandas warning. Since it's commented python code, it lights up with
|
Use case: from a_module import ( # noqa: I001
# functions
do_something, # noqa: I001
do_something_else, # noqa: I001
# variables
some_module_config_var, # noqa: I001
some_other_module_config_var,# noqa: I001
fifty_more_of_these, # fifty more noqa: I001
) because I wasn't doing that awfulness, and there's no way to disable this rule for a block of code, now my code has no import sorting. |
@AntiSol You shouldn't require to specify |
@dhruvmanila I actually tried that before posting. I don't think you're running the checker there? just the formatter? Isort rules are applied by the checker, not the formatter. I don't see any way to run the checker / isort rules there? You'll note that removing the noqa comment from your example has no effect on the formatter output |
Correct me if I'm wrong but I'm assuming that you're trying to ignore the names in that import statement from being sorted, right? Screen.Recording.2024-08-30.at.13.50.11.movThe video shows how to use code actions to apply a fix from the rules in the playground. |
No, I'm not right-clicking in the video. I'm just hovering over the squiggly lines and the pop-up will appear and then I'm clicking on the "Quick Fix" action which displays the code actions and then you can select any that's available.
The checker runs automatically in the playground every time the content changes, so there's no need to run it manually. It's not a bug, it's a feature :) |
@dhruvmanila right you are on all counts! And I was also able to confirm that this does indeed solve my issue, so you can just ignore all my comments on this thread. Thank you muchly! :) |
First of all, thank you so much for this tool, it's fantastic 🚀 !
I would like to request the possibility to support ignoring a block of code. To my surprise, this is a feature some people have needed as well from flake8, see SO question, though it is not supported.
In my current use case, I have a bunch of templates, that are too long, and it becomes cumbersome to add
noqa
to all of them.I was wondering if adding something like this would be possible (maybe with a different syntax):
Suggested changes
Thanks!
The text was updated successfully, but these errors were encountered: