Add allow-mixed-uninlined-format-args config #9865
Conversation
|
r? @xFrednet (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Hey @nyurik, thank you for the PR! I've seen it, this is just a small warning, that the review will take some time due to its size. I'll try to get it done by Thursday, but it might take until the end of next weekend. I hope that's alright with you :) |
|
☔ The latest upstream changes (presumably #9924) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@xFrednet Let me know if there are any blockers on this one. BTW, note that the actual code change is just a few lines - the rest is just a copy/paste of an integration test - to see that the same test behaves properly with a different configuration parameter. |
|
There are no blockers, just a busy week on my end and I got a cold. My schedule is mostly free on the weekend. You'll have a review the latest on Sunday. Thank you for your patience. :) |
|
Thanks @xFrednet, didn't mean to push you on this :) Get well soon! |
|
☔ The latest upstream changes (presumably #9945) made this pull request unmergeable. Please resolve the merge conflicts. |
nyurik
force-pushed
the
allow-mixed
branch
2 times, most recently
from
d5ab0ca
to
029714a
Compare
clippy_lints/src/format_args.rs
Outdated
| @@ -305,11 +321,23 @@ fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_si | |||
| Applicability::MachineApplicable, | |||
| if multiline_fix { CompletelyHidden } else { ShowCode }, | |||
| ); | |||
| if ignore_mixed { | |||
| // Improve lint config discoverability | |||
| diag.note_once( | |||
There was a problem hiding this comment.
I'm unsure about this part. Configurations are currently hard to discover, and discussing this should be on our agenda. A solution could be these notes, like you also suggested in #9880. However, until that has been discussed (Most likely on Tuesday on Zulip) I would hold off on this one.
Even then, I'd recommend adding the message all at once, to ensure that they're the same/similar. If this should be included in this PR, we should try to find a shorter wording. I don't remember if that's an official guideline, or just one we follow, but messages should ideally be shorter than 80 characters, 100 would also be fine, but this is a bit too long IMO.
We could remove the reference to the clippy.toml file, just mention that there is a configuration which is described in the lint description or say that this message could be suppressed via the XYZ config value. All of these are viable options with different tradeoffs. Deciding on a phrasing, is one of the reasons why I would like to do this as part of #9880 and not here.
tests/ui-toml/allow_mixed_uninlined_format_args/uninlined_format_args.rs
Show resolved
Hide resolved
|
☔ The latest upstream changes (presumably #9860) made this pull request unmergeable. Please resolve the merge conflicts. |
Implement `allow-mixed-uninlined-format-args` config param to change the behavior of the `uninlined_format_args` lint. Now it is a part of `style`, and won't propose inlining in case of a mixed usage, e.g. `print!("{} {}", var, 1+2)`. If the user sets allow-mixed-uninlined-format-args config param to `false`, then it would behave like before, proposing to inline args even in the mixed case.
|
@xFrednet thanks, makes sense, i think I addressed all of your feedback points. Thx again! |
|
Looks good to me, thank you very much for the addition! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Implement
allow-mixed-uninlined-format-argsconfig param to change the behavior of theuninlined_format_argslint. Now it is a part ofstyleper Zulip chat, and won't propose inlining in case of a mixed usage, e.g.print!("{} {}", var, 1+2). If the user setsallow-mixed-uninlined-format-argsconfig param tofalse, the lint would behave like it did before -- proposing to inline args even in the mixed case.changelog: [
uninlined_format_args]: Added a new configallow-mixed-uninlined-format-argsto allow the lint, if only some arguments can be inlined#9865
changelog: Moved [
uninlined_format_args] tostyle(Now warn-by-default)#9865