Allow human-readable names in rule selectors - #25887
Conversation
|
MichaReiser
left a comment
There was a problem hiding this comment.
I'm leaning towards splitting this PR and I even suggest to go one step further than what you did, by fully deferring rule parsing.
ty accepts all rule names on the CLI or in configurations. They're only validated when turning the configuration into a settings struct and ty emits a warning for every unknown rule.
#14443 is a ruff issue that asks for the same.
That's why I wonder what it would mean if we fix #14443 first by using an UnresolvedRuleName(String, source?) and only parse the rule to a RuleSelector in into_settings. I believe this should also allow you to merge the RuleSelector::Rule variants again.
Do you want to give this a try. I'm sure there's something that will make this unreasonably hard :)
|
Sure, I can give that a try! |
Summary -- This PR fixes #14443 by deferring rule parsing. This allows deserializing configuration files and CLI arguments with unknown rule selectors and enables us to emit a warning instead of a hard error when encountering an unknown selector. This will also help with #25887 because we can also defer parsing until we have an accurate `preview` setting for checking whether human-readable names should be allowed. I've left some of this infrastructure in place in this PR, with the `preview` argument currently unused. Test Plan -- Existing tests updated to show warnings instead of errors
check preview in other cases warn everywhere test all the flags tidy link to this pr on preview feature the per-file-ignore changes especially should be fully reverted
9d4018f to
672f9b6
Compare
There was a problem hiding this comment.
This is not strictly necessary, it just gives a nicer conversion than this awkward parsing round trip from the previous commit:
ruff/crates/ruff_linter/src/rule_selector.rs
Lines 26 to 28 in ff5b9cd
| When [preview mode](preview.md) is enabled, rule selectors also accept the human-readable name of a | ||
| rule (e.g., `unused-import`). |
There was a problem hiding this comment.
Codex wanted to make this much more prominent in the CLI and other docs, but I thought that seemed a bit premature and that this was a better starting point.
MichaReiser
left a comment
There was a problem hiding this comment.
Nice, this looks much "simpler" now.
I suggest we change RuleSelector::Rule to store a rule now, where we already resolve it anyway.
Do we need to update options.rs and mention that human-readable names are now accepted in many places under preview?
| let prefix = get_prefix_ident(&code.value()); | ||
| rule_selector_match_arms.extend(quote! { | ||
| #(#attrs)* Rule::#rule_name => RuleCodePrefix::#linter(#linter::#prefix), | ||
| }); |
There was a problem hiding this comment.
Instead of adding another match over all codes, could we change RuleSelector::Rule to store a Rule instead? I'm not sure what the reason was originally for storing only the selector prefix. But it might just have been because we used RuleSelector for deserialization, which we now no longer do.
There was a problem hiding this comment.
Yeah, this was very doable. Thanks!
|
|
||
| ----- stderr ----- | ||
| ruff failed | ||
| Cause: Rule name `unused-import` used as selector with preview disabled in `select` from [TMP]/ruff.toml |
There was a problem hiding this comment.
| Cause: Rule name `unused-import` used as selector with preview disabled in `select` from [TMP]/ruff.toml | |
| Cause: Rule name `unused-import` used as selector with preview disabled in `select` from `[TMP]/ruff.toml` |
|
|
||
| ----- stderr ----- | ||
| ruff failed | ||
| Cause: Rule name `unused-import` used as selector with preview disabled in `select` from the CLI |
There was a problem hiding this comment.
I find this a bit hard to understand. Maybe something like? Invalid --select unused-import. Selecting rules by their name requires enabling preview mode.
There was a problem hiding this comment.
I like the look of --select unused-import, but I don't think we can use --select since we also consider --config select=... as coming from the CLI. I like moving the preview part to the end, though. I went with:
Invalid selector `unused-import` in `select` from the CLI. Selecting rules by name requires preview mode
As I mentioned in my review comment, I thought it might be better to leave this out for now in favor of the single |
Summary
This PR enables using rule names as selectors in preview by also attempting to parse a selector as a human-readable name in
UnresolvedRuleSelector::resolve. If this succeeds and preview is enabled, the rule is activated, and if preview is disabled you get a custom error message.Test Plan
A few new CLI tests. I figured the tests from #26113 cover the shared behavior for the other selectors well enough and just focused on
selectas an example for both the CLI and config file.