[ruff] Add rule to use human-readable names in ruff:ignore comments (RUF106) - #26682
Conversation
|
…ts (`RUF106`) Summary -- This is the second of our migration rules for moving from `noqa` comments to `ruff:ignore` comments with human-readable names. `RUF106` performs the second part of this transformation, converting `ruff:ignore` comments that use rule codes to `ruff:ignore` comments with human-readable names. The rule skips unknown codes, external and otherwise. Unlike `RUF105` I don't think there's anything sensible we could do with those here. Test Plan -- New mdtests and a CLI test for the combination of `RUF105` and `RUF106`.
d692a79 to
68ce969
Compare
MichaReiser
left a comment
There was a problem hiding this comment.
This implementation is surprisingly simple.
I suggest changing the diagnostic range. Highlighting the entire ignore seems wrong to me:
- There's nothing wrong with the ignore itself, it's the codes
- There's nothing wrong with the reason
- There's nothing wrong with human-readable names within an ignore, but they're highlighted regardless.
| --> src/mdtest_snippet.py:2:1 | ||
| | | ||
| 2 | # ruff:ignore[F401, undefined-name, EXT001, UNKNOWN, F841] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
It would be nice if the rule only highlights the entire ignore if all codes are rule-codes. Otherwise, it would highlight consecutive runs of names.
However, I'd also be okay if we emit a separate diagnostic for each code. That's probably easiest. Given that this rule is primarily about the codes and not the ignore itself, I think that would be entirely reasoanble.
The reason why I think that'd be nicer is that it's not the entire ignore that's incorrect. You could also have a mixture of human-readable names and rule codes, in which case highlighting the entire ignore seems wrong to me
There was a problem hiding this comment.
I think I'll go for one diagnostic per code. That was actually my first instinct, but I went for this approach instead after seeing how RUF102 worked.
I do think the common case will be all codes, probably immediately after RUF105 runs on a noqa comment, but I still like separate diagnostics. I assume most noqa comments only have 1 code, in which case it's not that big of a difference either way.
Including the reason field was definitely an oversight in any case!
| --> src/mdtest_snippet.py:2:26 | ||
| | | ||
| 2 | value = 1 # explanation # ruff:ignore[F401, F841] reason # another | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
I'd find it surprising that reason is highlighted here. There's nothing inherently wrong with the reason. I think we should just highlight the codes. We can try to group consecutive runs of codes, but I don't think we should highlight the entire ignore
| Comments containing only names and external or unknown codes are unchanged: | ||
|
|
||
| ```py | ||
| # ruff:ignore[unused-import, EXT001, UNKNOWN] |
There was a problem hiding this comment.
Can you add a test for an ignore that uses human readable names and codes? I don't think this is covered yet
There was a problem hiding this comment.
I think this is covered by:
if I understand your suggestion. Or do you mean just human-readable names and valid codes, without the external codes?
There was a problem hiding this comment.
yes, I mean one with valid human readable names and valid codes.
There was a problem hiding this comment.
Added a version without the external codes:
Summary
This is the second of our migration rules for moving from
noqacomments toruff:ignorecommentswith human-readable names.
RUF106performs the second part of this transformation, convertingruff:ignorecomments that use rule codes toruff:ignorecomments with human-readable names.The rule skips unknown codes, external and otherwise. Unlike
RUF105I don't think there's anythingsensible we could do with those here.
Test Plan
New mdtests and a CLI test for the combination of
RUF105andRUF106.I did a local grep of our ecosystem projects and didn't find any
ruff:ignore-style suppressions, so I don't expect any ecosystem hits for this.