Customize where the fix_title sub-diagnostic appears#23044
Conversation
Summary -- This is a more general alternative to #23043 that more closely follows Micha's [suggestion]. I sort of prefer this approach because it gives the diagnostic author full control of the order of sub-diagnostics. On the other hand, it feels a bit too "clever" for the current usage, which achieves the same effect as the simpler #23043. So I'm curious to get others' thoughts. I still think the ideal solution long-term would be to combine the diff rendering with the sub-diagnostic holding the fix title, but that would be much more involved than either of these PRs. As I noted in the summary for #23043, the main motivation for the defer machinery is that [RUF064] has a bunch of early returns mixed in with its `info` additions, so it was a big pain to add a `help` diagnostic before each return. We may at least want a `deferred_help` helper instead of the two closures used here, if we do take this approach. [suggestion]: https://github.com/astral-sh/ruff/pull/19900/changes/BASE..ab685c16a8c01bb33f054698d2f32bb023d07429#r2276500131 [RUF064]: https://github.com/astral-sh/ruff/blob/4a32a96740bb7327a3f554b7f6d6675cb2ea797e/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs#L120-L145 Test Plan -- Updated snapshots for `RUF064` and `ISC004`.
|
conceptually, I think I'm in favor of the other PR for being more explicit about what it does, with less effort on the part of the rule to make that happen. |
|
Depending on your perspective, this PR is actually more explicit because the diagnostic author decides where to put the fix title instead of it implicitly appearing at the end. You can also just attach the fix title directly without deferring it (I just pushed that change to ISC004). Doing that everywhere |
|
Sorry, I guess I meant "more consistent" rather than "more explicit" |
|
(just going to mark this ready for review since CI doesn't seem to be cooperating) |
| diagnostic.before_drop(move |diag| { | ||
| if let Some(fix_title) = &fix_title { | ||
| diag.help(fix_title); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I might be missing something, but I think the formatter worked here. It does occasionally get confused with long lambdas, but I think this just looks funny because of the wrapped assignment on the line above.
There was a problem hiding this comment.
lol yep, my brain interpreted the semicolon as a curly brace 🤦♀️
| } | ||
| } | ||
|
|
||
| type BeforeDropFn = Box<dyn Fn(&mut Diagnostic)>; |
There was a problem hiding this comment.
Does this need to be declared here if it's only used in one place, or would it be better to inline it below?
There was a problem hiding this comment.
There was a clippy lint for this, otherwise I would have left it in place. I could just use an expect attribute but I didn't really mind pulling out the type alias.
There was a problem hiding this comment.
Yeah I think Clippy is too aggressive about this lol. It's just a boxed closure!
|
| } | ||
| } | ||
|
|
||
| type BeforeDropFn = Box<dyn Fn(&mut Diagnostic)>; |
There was a problem hiding this comment.
Yeah I think Clippy is too aggressive about this lol. It's just a boxed closure!
| /// sub-diagnostics. For example: | ||
| /// | ||
| /// ```ignore | ||
| /// let (mut diagnostic, fix_title) = checker.report_custom_diagnostic(MyViolation, range); |
There was a problem hiding this comment.
Is there ever a case where you get a fix_title back but don't want to add it via before_drop? Just wondering if checker.report_custom_diagnostic should wire that up for you?
There was a problem hiding this comment.
Oh yeah, good point. I guess even if you did want to push a diagnostic, then attach the fix title, then another diagnostic, you could just defer that last diagnostic too. That should really simplify the API, thank you!
3 participants
Summary
This is a more general alternative to #23043 that more closely follows Micha's
suggestion. I sort of prefer this approach because it gives the diagnostic
author full control of the order of sub-diagnostics. On the other hand, it feels
a bit too "clever" for the current usage, which achieves the same effect as the
simpler #23043. So I'm curious to get others' thoughts.
I still think the ideal solution long-term would be to combine the diff
rendering with the sub-diagnostic holding the fix title, but that would be much
more involved than either of these PRs.
As I noted in the summary for #23043, the main motivation for the defer
machinery is that RUF064 has a bunch of early returns mixed in with its
infoadditions, so it was a big pain to add a
helpdiagnostic before each return.We may at least want a
deferred_helphelper instead of the closure usedhere, if we do take this approach.
Test Plan
Updated snapshots for
RUF064andISC004.