Skip to content

Commit 155fd60

Browse files
ntBreMeGaGiGaGon
andauthored
Document when a rule was added (#21035)
Summary -- Inspired by #20859, this PR adds the version a rule was added, and the file and line where it was defined, to `ViolationMetadata`. The file and line just use the standard `file!` and `line!` macros, while the more interesting version field uses a new `violation_metadata` attribute parsed by our `ViolationMetadata` derive macro. I moved the commit modifying all of the rule files to the end, so it should be a lot easier to review by omitting that one. As a curiosity and a bit of a sanity check, I also plotted the rule numbers over time: <img width="640" height="480" alt="image" src="https://github.com/user-attachments/assets/75b0b5cc-3521-4d40-a395-8807e6f4925f" /> I think this looks pretty reasonable and avoids some of the artifacts the earlier versions of the script ran into, such as the `rule` sub-command not being available or `--explain` requiring a file argument. <details><summary>Script and summary data</summary> ```shell gawk --csv ' NR > 1 { split($2, a, ".") major = a[1]; minor = a[2]; micro = a[3] # sum the number of rules added per minor version versions[minor] += 1 } END { tot = 0 for (i = 0; i <= 14; i++) { tot += versions[i] print i, tot } } ' ruff_rules_metadata.csv > summary.dat ``` ``` 0 696 1 768 2 778 3 803 4 822 5 848 6 855 7 865 8 893 9 915 10 916 11 924 12 929 13 932 14 933 ``` </details> Test Plan -- I built and viewed the documentation locally, and it looks pretty good! <img width="1466" height="676" alt="image" src="https://github.com/user-attachments/assets/5e227df4-7294-4d12-bdaa-31cac4e9ad5c" /> The spacing seems a bit awkward following the `h1` at the top, so I'm wondering if this might look nicer as a footer in Ruff. The links work well too: - [v0.0.271](https://github.com/astral-sh/ruff/releases/tag/v0.0.271) - [Related issues](https://github.com/astral-sh/ruff/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20airflow-variable-name-task-id-mismatch) - [View source](https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fairflow%2Frules%2Ftask_variable_name.rs#L34) The last one even works on `main` now since it points to the `derive(ViolationMetadata)` line. In terms of binary size, this branch is a bit bigger than main with 38,654,520 bytes compared to 38,635,728 (+20 KB). I guess that's not _too_ much of an increase, but I wanted to check since we're generating a lot more code with macros. --------- Co-authored-by: GiGaGon <[email protected]>
1 parent 48f1771 commit 155fd60

File tree

703 files changed

+2107
-1007
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

703 files changed

+2107
-1007
lines changed

crates/ruff/src/commands/rule.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ struct Explanation<'a> {
2525
explanation: Option<&'a str>,
2626
preview: bool,
2727
status: RuleGroup,
28+
source_location: SourceLocation,
2829
}
2930

3031
impl<'a> Explanation<'a> {
@@ -43,6 +44,10 @@ impl<'a> Explanation<'a> {
4344
explanation: rule.explanation(),
4445
preview: rule.is_preview(),
4546
status: rule.group(),
47+
source_location: SourceLocation {
48+
file: rule.file(),
49+
line: rule.line(),
50+
},
4651
}
4752
}
4853
}
@@ -127,3 +132,14 @@ pub(crate) fn rules(format: HelpFormat) -> Result<()> {
127132
}
128133
Ok(())
129134
}
135+
136+
/// The location of the rule's implementation in the Ruff source tree, relative to the repository
137+
/// root.
138+
///
139+
/// For most rules this will point to the `#[derive(ViolationMetadata)]` line above the rule's
140+
/// struct.
141+
#[derive(Serialize)]
142+
struct SourceLocation {
143+
file: &'static str,
144+
line: u32,
145+
}

crates/ruff/tests/integration_test.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -953,7 +953,11 @@ fn rule_f401() {
953953

954954
#[test]
955955
fn rule_f401_output_json() {
956-
assert_cmd_snapshot!(ruff_cmd().args(["rule", "F401", "--output-format", "json"]));
956+
insta::with_settings!({filters => vec![
957+
(r#"("file": ")[^"]+(",)"#, "$1<FILE>$2"),
958+
]}, {
959+
assert_cmd_snapshot!(ruff_cmd().args(["rule", "F401", "--output-format", "json"]));
960+
});
957961
}
958962

959963
#[test]

crates/ruff/tests/snapshots/integration_test__rule_f401_output_json.snap

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,14 @@ exit_code: 0
2525
"fix_availability": "Sometimes",
2626
"explanation": "## What it does\nChecks for unused imports.\n\n## Why is this bad?\nUnused imports add a performance overhead at runtime, and risk creating\nimport cycles. They also increase the cognitive load of reading the code.\n\nIf an import statement is used to check for the availability or existence\nof a module, consider using `importlib.util.find_spec` instead.\n\nIf an import statement is used to re-export a symbol as part of a module's\npublic interface, consider using a \"redundant\" import alias, which\ninstructs Ruff (and other tools) to respect the re-export, and avoid\nmarking it as unused, as in:\n\n```python\nfrom module import member as member\n```\n\nAlternatively, you can use `__all__` to declare a symbol as part of the module's\ninterface, as in:\n\n```python\n# __init__.py\nimport some_module\n\n__all__ = [\"some_module\"]\n```\n\n## Preview\nWhen [preview] is enabled (and certain simplifying assumptions\nare met), we analyze all import statements for a given module\nwhen determining whether an import is used, rather than simply\nthe last of these statements. This can result in both different and\nmore import statements being marked as unused.\n\nFor example, if a module consists of\n\n```python\nimport a\nimport a.b\n```\n\nthen both statements are marked as unused under [preview], whereas\nonly the second is marked as unused under stable behavior.\n\nAs another example, if a module consists of\n\n```python\nimport a.b\nimport a\n\na.b.foo()\n```\n\nthen a diagnostic will only be emitted for the first line under [preview],\nwhereas a diagnostic would only be emitted for the second line under\nstable behavior.\n\nNote that this behavior is somewhat subjective and is designed\nto conform to the developer's intuition rather than Python's actual\nexecution. To wit, the statement `import a.b` automatically executes\n`import a`, so in some sense `import a` is _always_ redundant\nin the presence of `import a.b`.\n\n\n## Fix safety\n\nFixes to remove unused imports are safe, except in `__init__.py` files.\n\nApplying fixes to `__init__.py` files is currently in preview. The fix offered depends on the\ntype of the unused import. Ruff will suggest a safe fix to export first-party imports with\neither a redundant alias or, if already present in the file, an `__all__` entry. If multiple\n`__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix\nto remove third-party and standard library imports -- the fix is unsafe because the module's\ninterface changes.\n\nSee [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc)\nfor more details on how Ruff\ndetermines whether an import is first or third-party.\n\n## Example\n\n```python\nimport numpy as np # unused import\n\n\ndef area(radius):\n return 3.14 * radius**2\n```\n\nUse instead:\n\n```python\ndef area(radius):\n return 3.14 * radius**2\n```\n\nTo check the availability of a module, use `importlib.util.find_spec`:\n\n```python\nfrom importlib.util import find_spec\n\nif find_spec(\"numpy\") is not None:\n print(\"numpy is installed\")\nelse:\n print(\"numpy is not installed\")\n```\n\n## Options\n- `lint.ignore-init-module-imports`\n- `lint.pyflakes.allowed-unused-imports`\n\n## References\n- [Python documentation: `import`](https://docs.python.org/3/reference/simple_stmts.html#the-import-statement)\n- [Python documentation: `importlib.util.find_spec`](https://docs.python.org/3/library/importlib.html#importlib.util.find_spec)\n- [Typing documentation: interface conventions](https://typing.python.org/en/latest/spec/distributing.html#library-interface-public-and-private-symbols)\n\n[preview]: https://docs.astral.sh/ruff/preview/\n",
2727
"preview": false,
28-
"status": "Stable"
28+
"status": {
29+
"Stable": {
30+
"since": "v0.0.18"
31+
}
32+
},
33+
"source_location": {
34+
"file": "<FILE>",
35+
"line": 145
36+
}
2937
}
3038
----- stderr -----

crates/ruff_dev/src/generate_docs.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use std::path::PathBuf;
88
use anyhow::Result;
99
use itertools::Itertools;
1010
use regex::{Captures, Regex};
11+
use ruff_linter::codes::RuleGroup;
1112
use strum::IntoEnumIterator;
1213

1314
use ruff_linter::FixAvailability;
@@ -31,6 +32,47 @@ pub(crate) fn main(args: &Args) -> Result<()> {
3132

3233
let _ = writeln!(&mut output, "# {} ({})", rule.name(), rule.noqa_code());
3334

35+
let status_text = match rule.group() {
36+
RuleGroup::Stable { since } => {
37+
format!(
38+
r#"Added in <a href="https://github.com/astral-sh/ruff/releases/tag/{since}">{since}</a>"#
39+
)
40+
}
41+
RuleGroup::Preview { since } => {
42+
format!(
43+
r#"Preview (since <a href="https://github.com/astral-sh/ruff/releases/tag/{since}">{since}</a>)"#
44+
)
45+
}
46+
RuleGroup::Deprecated { since } => {
47+
format!(
48+
r#"Deprecated (since <a href="https://github.com/astral-sh/ruff/releases/tag/{since}">{since}</a>)"#
49+
)
50+
}
51+
RuleGroup::Removed { since } => {
52+
format!(
53+
r#"Removed (since <a href="https://github.com/astral-sh/ruff/releases/tag/{since}">{since}</a>)"#
54+
)
55+
}
56+
};
57+
58+
let _ = writeln!(
59+
&mut output,
60+
r#"<small>
61+
{status_text} ·
62+
<a href="https://github.com/astral-sh/ruff/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20(%27{encoded_name}%27%20OR%20{rule_code})" target="_blank">Related issues</a> ·
63+
<a href="https://github.com/astral-sh/ruff/blob/main/{file}#L{line}" target="_blank">View source</a>
64+
</small>
65+
66+
"#,
67+
encoded_name =
68+
url::form_urlencoded::byte_serialize(rule.name().as_str().as_bytes())
69+
.collect::<String>(),
70+
rule_code = rule.noqa_code(),
71+
file =
72+
url::form_urlencoded::byte_serialize(rule.file().replace('\\', "/").as_bytes())
73+
.collect::<String>(),
74+
line = rule.line(),
75+
);
3476
let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap();
3577
if linter.url().is_some() {
3678
let common_prefix: String = match linter.common_prefix() {

crates/ruff_dev/src/generate_rules_table.rs

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,20 +32,24 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
3232
table_out.push('\n');
3333
for rule in rules {
3434
let status_token = match rule.group() {
35-
RuleGroup::Removed => {
35+
RuleGroup::Removed { since } => {
3636
format!(
37-
"<span {SYMBOL_STYLE} title='Rule has been removed'>{REMOVED_SYMBOL}</span>"
37+
"<span {SYMBOL_STYLE} title='Rule was removed in {since}'>{REMOVED_SYMBOL}</span>"
3838
)
3939
}
40-
RuleGroup::Deprecated => {
40+
RuleGroup::Deprecated { since } => {
4141
format!(
42-
"<span {SYMBOL_STYLE} title='Rule has been deprecated'>{WARNING_SYMBOL}</span>"
42+
"<span {SYMBOL_STYLE} title='Rule has been deprecated since {since}'>{WARNING_SYMBOL}</span>"
4343
)
4444
}
45-
RuleGroup::Preview => {
46-
format!("<span {SYMBOL_STYLE} title='Rule is in preview'>{PREVIEW_SYMBOL}</span>")
45+
RuleGroup::Preview { since } => {
46+
format!(
47+
"<span {SYMBOL_STYLE} title='Rule has been in preview since {since}'>{PREVIEW_SYMBOL}</span>"
48+
)
49+
}
50+
RuleGroup::Stable { since } => {
51+
format!("<span {SYMBOL_STYLE} title='Rule has been stable since {since}'></span>")
4752
}
48-
RuleGroup::Stable => format!("<span {SYMBOL_STYLE}></span>"),
4953
};
5054

5155
let fix_token = match rule.fixable() {

0 commit comments

Comments
 (0)