Implement many-to-one mapping between codes and rules #2517
Conversation
not-my-profile
force-pushed
the
many-to-one
branch
2 times, most recently
from
6dd9837 to
94cbacf
Compare
|
This is really exciting. I've read through some of the code and commits but not the entirety of the change (yet). From my testing, it seems like every rule has one "primary" code. So |
|
I'm not sure what I would expect as a user. I could imagine a few things:
I don't know how I'd expect |
Yes that's how it currently works. I'd expect us to switch to human-friendly rule names #1773, so no matter which code you used to select in the future ruff will always only report the human-friendly rule name. Until we make that switch the behavior will be a bit confusing, but I don't see a good way around that.
I am positive that we absolutely do not want to report any error multiple times under any circumstance. Note that it's totally possible that a rule is enabled via multiple codes, I don't think we want to report multiple codes for a single violation, so we have to pick one ... until we have come up with rule naming guidelines and renamed our rules accordingly.
Yes. A rule can be identified via multiple codes. Which code you used to enable a rule doesn't matter. |
|
I worry that running |
not-my-profile
force-pushed
the
many-to-one
branch
6 times, most recently
from
c7f549a to
00b2bd7
Compare
|
I'm hesitant to merge this as-is due to some of the confusing behaviors that users will experience around aliasing (e.g., the I don't have great answers for them yet, but I know that if we ship this as-is, it will be confusing for users, and we'll get a lot of feedback and questions stemming from that confusion. If we want to merge this while giving ourselves time to figure out the best solution for aliasing, what we could do is merge this change but not yet implement any of the actual aliases (apart from, perhaps, deprecating rules, like |
|
Right that makes sense. I think the course of action is:
I already rebased this PR yesterday, which was quite work-intensive since #2583 had landed in between. Further rebasing shouldn't take that much effort, so I'd be alright with leaving this PR open ... but yeah it would be nice to merge this just so that I don't have to deal with merging other changes that may crop up. I could add an assert statement to the Footnotes
|
|
(Heads-up: I will rename the pathlib rules in #2348 to os-path - the thing we detect) |
|
I think that landing this PR, will take some coordination that this will be merged before any other |
We want to remove the variants denoting whole Linters from the RuleCodePrefix enum, so we have to introduce a new RuleSelector::Linter variant.
Post this commit series several codes can be mapped to a single rule, this commit therefore renames Rule::code to Rule::noqa_code, which is the code that --add-noqa will add to ignore a rule.
|
👍 I’m happy to merge this next assuming it doesn’t change the UX right now — is that the case? I have to re-read the code but I’ll review tomorrow after the JetBrains webinar, and hopefully can merge tomorrow afternoon or evening. |
|
Ok great. Yes after I have dropped the last commit demonstrating the mapping and added the aforementioned assert statement there shouldn't be any UX changes. |
Rule::noqa_code previously return a single &'static str, which was possible because we had one enum listing all rule code prefixes. This commit series will however split up the RuleCodePrefix enum into several enums ... so we'll end up with two &'static str ... this commit wraps the return type of Rule::noqa_code into a newtype so that we can easily change it to return two &'static str in the 6th commit of this series.
Same reasoning as for the previous commit ... one &'static str becomes two &'static str because we split the RuleCodePrefix enum. Note that the .unwrap() we have to add now, will actually be removed in the 6th commit.
Currently the define_rule_mapping! macro generates both the Rule enum as well as the RuleCodePrefix enum and the mapping between the two. After this commit series the macro will only generate the Rule enum and the RuleCodePrefix enum and the mapping will be generated by a new map_codes proc macro, so we rename the macro now to fit its new purpose.
# This commit was generated by running the following Python code:
# (followed by `sed -Ei 's/(mod registry;)/\1mod codes;/' crates/ruff/src/lib.rs`
# and `cargo fmt`).
import json
import re
import subprocess
def parse_registry():
file = open('crates/ruff/src/registry.rs')
rules = []
while next(file) != 'ruff_macros::register_rules!(\n':
continue
while (line := next(file)) != ');\n':
line = line.strip().rstrip(',')
if line.startswith('//') or line.startswith('#['):
rules.append(line)
continue
code, path = line.split(' => ')
name = path.rsplit('::')[-1]
rules.append((code, name))
while (line := next(file)) != 'pub enum Linter {\n':
continue
prefixes = []
prefix2linter = []
while (line := next(file).strip()) != '}':
if line.startswith('//'):
continue
if line.startswith('#[prefix = '):
prefixes.append(line.split()[-1].strip('"]'))
else:
for prefix in prefixes:
prefix2linter.append((prefix, line.rstrip(',')))
prefixes.clear()
prefix2linter.sort(key = lambda t: len(t[0]), reverse=True)
return rules, prefix2linter
rules, prefix2linter = parse_registry()
def parse_code(code):
prefix = re.match('[A-Z]+', code).group()
if prefix in ('E', 'W'):
return 'Pycodestyle', code
for prefix, linter in prefix2linter:
if code.startswith(prefix):
return linter, code[len(prefix) :]
assert False
text = '''
use crate::registry::{Linter, Rule};
pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
#[allow(clippy::enum_glob_use)]
use Linter::*;
Some(match (linter, code) {
'''
for entry in rules:
if isinstance(entry, str):
if entry.startswith('//'):
text += '\n' + entry
else:
text += entry
else:
namespace, code = parse_code(entry[0])
text += f'({namespace}, "{code}") => Rule::{entry[1]},'
text += '\n'
text += '''
_ => return None,
})
}
'''
with open('crates/ruff/src/codes.rs', 'w') as f:
f.write(text)
This commit was generated by running:
fastmod --accept-all '[A-Z]+[0-9]+ => ' '' crates/ruff/src/registry.rs
not-my-profile
force-pushed
the
many-to-one
branch
from
00b2bd7 to
9b92e55
Compare
not-my-profile
force-pushed
the
many-to-one
branch
from
9b92e55 to
fdc71d0
Compare
|
(Returning to this now.) |
|
Let me know if you have questions. |
|
Cool this looks good to me -- merging... |
Implements #2186.