[ruff] Expand lint.external docs and add sub-diagnostic (RUF100, RUF102)#23268
[ruff] Expand lint.external docs and add sub-diagnostic (RUF100, RUF102)#23268
ruff] Expand lint.external docs and add sub-diagnostic (RUF100, RUF102)#23268Conversation
…, `RUF102`) Summary -- We've gotten a couple of recent reports about suppressing both of these rules (#23191 and #23267 at least), so I think it's worth trying to provide more information in the diagnostics and also in the documentation. The connection to `lint.external` was at least documented, but the rule docs themselves didn't really help unless you clicked through to the setting itself. I'm not 100% sold on this sub-diagnostic message. It feels a bit verbose, but I like that it's explicit. Given full control over the placement, I'd probably put it below the fix diff, but this seems like the best option we currently have available. Test Plan -- Updated snapshot tests
|
amyreese
left a comment
There was a problem hiding this comment.
It looks like this is triggering on regular "unused noqa" instances as well. We probably also need to intentionally do this for RUF102 reported by the range suppression module too.
...uff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__range_suppressions.snap
Outdated
Show resolved
Hide resolved
|
We'll want to add matching logic for range suppressions here: ruff/crates/ruff_linter/src/suppression.rs Line 397 in 55d06c8 Something like if let InvalidRuleCode = kind {
// add help text here
} |
|
I don't believe we can match on the violation kind like that, but we can return the |
|
There's no way that this is the right way to format this code: + let process_pending_diagnostics = |key: Option<TextRange>,
+ grouped_diagnostic: &Option<(
+ TextRange,
+ SuppressionDiagnostic,
+ )>|
+ -> bool {https://github.com/astral-sh/ruff/actions/runs/22161202610/job/64078091120?pr=23268 I think I'll extract this closure into a function to avoid this, sorry for the big diff! Somehow rustfmt got really unhappy after the merge. |
Summary
We've gotten a couple of recent reports about suppressing both of these
rules (#23191 and #23267 at least), so I think it's worth trying to provide more
information in the diagnostics and also in the documentation. The connection to
lint.externalwas at least documented, but the rule docs themselves didn'treally help unless you clicked through to the setting itself.
I'm not 100% sold on this sub-diagnostic message. It feels a bit verbose, but I
like that it's explicit. Given full control over the placement, I'd probably put
it below the fix diff, but this seems like the best option we currently have
available.
Test Plan
Updated snapshot tests