Drop `RuleCode` in favor of `Rule` enum with human-friendly names #1941

not-my-profile · 2023-01-18T01:45:29Z

Rules often exist in several linters for example UselessObjectInheritance is known as:

UP004 in pyupgrade
R0205 in pylint
PIE792 in flake8-pie

Ruff currently only supports one code per rule (aside from "prefix redirects" which aren't documented at all), so basically the first code used ends up being the code promoted by ruff which is very arbitrary.

I think we should rather have a proper many to many mapping, allowing many codes to be mapped to one ruff rule or one code to several ruff rules (e.g. C0103 from pylint encompasses N801, N802 and N803 from pep8-naming). This would also allow flake8_to_ruff to be expanded to support e.g. pylint.

To cleanly implement this we have to decouple "rules" from "rule codes". This is the first PR for that epic task ... it doesn't yet change the mapping, only which variant names we use in our codebase.

Added benefits: The code is no longer littered with cryptic numeric codes and thus more readable and we get a big step closer to #1773.

charliermarsh · 2023-01-18T04:47:04Z

I haven't looked at the code yet, but I definitely agree with the argument put forward in the summary -- I've wanted this for a long time!

not-my-profile · 2023-01-18T08:46:19Z

Alright let me give you some more context: The PR changes nothing about the UI. The first two commits are solely preparatory and could be merged independently from the rest. I have relabeled the remaining 7 commits as {n}/7 to make it clear that they belong together and have amended the 1st of those commits to explain the commit set in the commit message.

The derive implementations that depend on the variant names are removed to guarantee that they aren't used by anything anymore so that the last commit doesn't break anything ... these derive implementations may very well be added back in the future.

As always I strongly advise you to review this commit by commit to make sense of the changes :)

charliermarsh · 2023-01-18T16:09:08Z

Thank you. I know it's likely tedious to have to keep it up-to-date so feel free to rebase at the end or as you like. I'll try to review this today -- bear with me as it's a large PR :)

charliermarsh · 2023-01-19T00:56:10Z

(Will review this tonight.)

Cargo.toml

charliermarsh · 2023-01-19T02:11:43Z

ruff_cli/src/commands.rs

@@ -114,7 +114,7 @@ pub fn run(
 .unwrap_or_else(|(path, message)| {
 if let Some(path) = &path {
 let settings = resolver.resolve(path, pyproject_strategy);
- if settings.rules.enabled(&Rule::E902) {
+ if settings.rules.enabled(&Rule::IOError) {


Did you change these manually, or via some automation?

I did the initial conversion automatically via a small Python script however that script isn't idempotent and the last commit also contained some manual changes so I have been manually rebasing since.

(I can try to make this the next thing we merge.)

Ok great let's do that ... I already addressed your two other comments locally ... then I'll start the final rebase ... doing it automatically this time because there have been too much changes to do this manually ... I'll have to split the last commit into two one for the manual changes and one for the automatic ones and update my python script^^.

Ok, all good. I'll hold off on merging anything until this is updated and in.

See the last commit message for the script I used to generate it (the last commit is now 100% autogenerated).

src/checkers/imports.rs

charliermarsh · 2023-01-19T02:16:59Z

ruff_cli/src/cli.rs

@@ -180,7 +180,7 @@ pub struct Cli {
 conflicts_with = "stdin_filename",
 conflicts_with = "watch",
 )]
- pub explain: Option<RuleCode>,
+ pub explain: Option<String>,


Maybe the one "loss" in this series of commits. I wish we could catch these statically as part of the CLI.

Separately, while we're here (and now that the type is string), can we set value_name = "RULE_CODE" for this?

I have changed this to Option<Rule> by setting the clap option value_parser=Rule::from_code.

can we set value_name = "RULE_CODE" for this?

I agree that this would be a good addition however this PR accomplishes one thing and I'd rather keep unrelated changes for other PRs.

All good, preserving the type is great.

charliermarsh · 2023-01-19T02:17:23Z

ruff_cli/src/main.rs

+ }
+ Err(error) => {
+ eprintln!("{error}");
+ return Ok(ExitCode::FAILURE);


Is this intended? It should all be handled by the outer function via commands::explain(&code, format)?;.

With the previously described change to use value_parser this diff has been removed.

This commit series refactors ruff to decouple "rules" from "rule codes", in order to: 1. Make our code more readable by changing e.g. RuleCode::UP004 to Rule::UselessObjectInheritance. 2. Let us cleanly map multiple codes to one rule, for example: [UP004] in pyupgrade, [R0205] in pylint and [PIE792] in flake8-pie all refer to the rule UselessObjectInheritance but ruff currently only associates that rule with the UP004 code (since the implementation was initially modeled after pyupgrade). 3. Let us cleanly map one code to multiple rules, for example: [C0103] from pylint encompasses N801, N802 and N803 from pep8-naming. The latter two steps are not yet implemented by this commit series but this refactoring enables us to introduce such a mapping. Such a mapping would also let us expand flake8_to_ruff to support e.g. pylint. After the next commit which just does some renaming the following four commits remove all trait derivations from the Rule (previously RuleCode) enum that depend on the variant names to guarantee that they are not used anywhere anymore so that we can rename all of these variants in the eigth and final commit without breaking anything. While the plan very much is to also surface these human-friendly names more in the user interface this is not yet done in this commit series, which does not change anything about the UI: it's purely a refactor. [UP004]: pyupgrade doesn't actually assign codes to its messages. [R0205]: https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/useless-object-inheritance.html [PIE792]: https://github.com/sbdchd/flake8-pie#pie792-no-inherit-object [C0103]: https://pylint.pycqa.org/en/latest/user_guide/messages/convention/invalid-name.html

# This commit was automatically generated by running the following # script (followed by `cargo +nightly fmt`): import glob import re from typing import NamedTuple class Rule(NamedTuple): code: str name: str path: str def rules() -> list[Rule]: """Returns all the rules defined in `src/registry.rs`.""" file = open('src/registry.rs') rules = [] while next(file) != 'ruff_macros::define_rule_mapping!(\n': continue while (line := next(file)) != ');\n': line = line.strip().rstrip(',') if line.startswith('//'): continue code, path = line.split(' => ') name = path.rsplit('::')[-1] rules.append(Rule(code, name, path)) return rules code2name = {r.code: r.name for r in rules()} for pattern in ('src/**/*.rs', 'ruff_cli/**/*.rs', 'ruff_dev/**/*.rs', 'scripts/add_*.py'): for name in glob.glob(pattern, recursive=True): with open(name) as f: text = f.read() text = re.sub('Rule(?:Code)?::([A-Z]\w+)', lambda m: 'Rule::' + code2name[m.group(1)], text) text = re.sub(r'(?<!"<FilePattern>:<)RuleCode\b', 'Rule', text) text = re.sub('(use crate::registry::{.*, Rule), Rule(.*)', r'\1\2', text) # fix duplicate import with open(name, 'w') as f: f.write(text)

not-my-profile · 2023-01-19T05:06:25Z

Thanks for reviewing and merging, I think this greatly improves the overall code readability. 🎉

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.226` -> `^0.0.227` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.227/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.227/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.227/compatibility-slim/0.0.226)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.227/confidence-slim/0.0.226)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.227`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.227) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.226...v0.0.227) #### What's Changed - Drop `RuleCode` in favor of `Rule` enum with human-friendly names by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1941 - Make define_rule_mapping! set rule code as doc comment of variants by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1991 - Added pylint formatter by [@damienallen](https://togithub.com/damienallen) in [astral-sh/ruff#1995 - Preserve unmatched comparators in `SIM109` by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1998 - Drop `Violation::placeholder` by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1996 - Apply #\[derive(Default)] fixes suggested by Clippy by [@akx](https://togithub.com/akx) in [astral-sh/ruff#2000 - Avoid `SIM201` and `SIM202` errors in `__ne__` et al by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2001 - Fix that --explain panics by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2002 - Split up pydocstyle rules by [@akx](https://togithub.com/akx) in [astral-sh/ruff#2003 - Add RUF005 "unpack instead of concatenating" check by [@akx](https://togithub.com/akx) in [astral-sh/ruff#1957 - Enable autofix for `FitsOnOneLine` (`D200`) by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2006 - Change AsRef<str> impl for Rule to kebab-case by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2009 - Upgrade RustPython by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2011 - Avoid SIM401 in `elif` blocks by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2012 - Improve --explain output by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2010 - Avoid checking row types for single-name [@parametrize](https://togithub.com/parametrize) decorators by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2013 #### New Contributors - [@damienallen](https://togithub.com/damienallen) made their first contribution in [astral-sh/ruff#1995 **Full Changelog**: astral-sh/ruff@v0.0.226...v0.0.227 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

not-my-profile force-pushed the rulecode-to-rule branch 5 times, most recently from a8030fd to ba2e0be Compare January 18, 2023 02:25

not-my-profile force-pushed the rulecode-to-rule branch 2 times, most recently from ee947df to b278cd6 Compare January 18, 2023 08:40

charliermarsh mentioned this pull request Jan 18, 2023

Possible overlap between SIM300 and PLC2201 #1954

Closed

charliermarsh reviewed Jan 19, 2023

View reviewed changes

not-my-profile force-pushed the rulecode-to-rule branch from b278cd6 to 7148ad6 Compare January 19, 2023 04:38

not-my-profile added 10 commits January 19, 2023 05:40

refactor: Make define_rule_mapping! generate RuleCodePrefix directly

9b5c904

refactor: Use Self:: in match arms

7b26756

rule 2/8: Rename DiagnosticKind::code to rule

6061b9c

rule 3/8: Remove AsRef<str> impl for Rule

daac14e

rule 4/8: Remove Display impl for Rule

c9d10c8

rule 5/8: Remove FromStr impl for Rule

10b5eea

rule 6/8: Remove Serialize & Deserialize impls for Rule

70e4f3d

rule 7/8: Change Rule enum definition

5a6c3e2

not-my-profile force-pushed the rulecode-to-rule branch from 7148ad6 to fa9592b Compare January 19, 2023 04:46

charliermarsh merged commit 6649225 into astral-sh:main Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop `RuleCode` in favor of `Rule` enum with human-friendly names #1941

Drop `RuleCode` in favor of `Rule` enum with human-friendly names #1941

not-my-profile commented Jan 18, 2023 •

edited

Loading

charliermarsh commented Jan 18, 2023

not-my-profile commented Jan 18, 2023 •

edited

Loading

charliermarsh commented Jan 18, 2023

charliermarsh commented Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

not-my-profile Jan 19, 2023

charliermarsh Jan 19, 2023

charliermarsh Jan 19, 2023

not-my-profile Jan 19, 2023

not-my-profile commented Jan 19, 2023

Drop RuleCode in favor of Rule enum with human-friendly names #1941

Drop RuleCode in favor of Rule enum with human-friendly names #1941

Conversation

not-my-profile commented Jan 18, 2023 • edited Loading

charliermarsh commented Jan 18, 2023

not-my-profile commented Jan 18, 2023 • edited Loading

charliermarsh commented Jan 18, 2023

charliermarsh commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-my-profile commented Jan 19, 2023

Drop `RuleCode` in favor of `Rule` enum with human-friendly names #1941

Drop `RuleCode` in favor of `Rule` enum with human-friendly names #1941

not-my-profile commented Jan 18, 2023 •

edited

Loading

not-my-profile commented Jan 18, 2023 •

edited

Loading