Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
25 changes: 20 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,13 @@ after running `cargo test` like so:
cargo insta review
```

If your pull request relates to a specific lint rule, include the category and rule code in the
title, as in the following examples:
If your pull request relates to a specific lint rule, include the category and rule code or name in
the title, as in the following examples:

- \[`flake8-bugbear`\] Avoid false positive for usage after `continue` (`B031`)
- \[`flake8-simplify`\] Detect implicit `else` cases in `needless-bool` (`SIM103`)
- \[`pycodestyle`\] Implement `redundant-backslash` (`E502`)
- \[`pedantic`\] Implement `pytest-fixture-autouse`

Your pull request will be reviewed by a maintainer, which may involve a few rounds of iteration
prior to merging.
Expand Down Expand Up @@ -225,7 +226,17 @@ adding a new lint rule are as follows:
statements, like imports) or `analyze/expression.rs` (if your rule is based on analyzing
expressions, like function calls).

1. Map the violation struct to a rule code in `crates/ruff_linter/src/codes.rs` (e.g., `B011`).
1. Register the violation struct in `crates/ruff_linter/src/codes.rs` (e.g., `B011`). If your lint
rule comes from an existing linter, you can map it to that linter and give it a code. Otherwise,
you can leave the linter and code blank, registering it only to a category. For example:

```rust
// Rules with linter groups and codes
(Flake8Logging, "015") => rules::flake8_logging::rules::RootLoggerCall,

// Rules with only a category
() => rules::ruff::rules::PytestFixtureAutouse,
```

1. Add proper [testing](#rule-testing-fixtures-and-snapshots) for your rule.

Expand All @@ -244,11 +255,15 @@ Once you're satisfied with your code, add tests for your rule
(see: [rule testing](#rule-testing-fixtures-and-snapshots)), and regenerate the documentation and
associated assets (like our JSON Schema) with `cargo dev generate-all`.

Finally, submit a pull request, and include the category, rule name, and rule code in the title, as
in:
Finally, submit a pull request, and include the category, rule name, and rule code (if applicable)
in the title, as in:

> \[`pycodestyle`\] Implement `redundant-backslash` (`E502`)

or

> \[`pedantic`\] Implement `pytest-fixture-autouse`

#### Rule testing: fixtures and snapshots

To test rules, Ruff uses the mdtest framework, initially developed for ty. Mdtests are written as
Expand Down
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/ruff/src/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ mod test {
}

// Configure
let snapshot = format!("{}_{}", rule_code.noqa_code(), path);
let snapshot = format!("{}_{}", rule_code.name(), path);
// invalid pyproject.toml is not active by default
let settings = Settings {
linter: LinterSettings::for_rules(vec![rule_code, Rule::InvalidPyprojectToml]),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/commands/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,7 @@ pub(super) fn warn_incompatible_formatter_settings(resolver: &Resolver) {
if !incompatible_rules.is_empty() {
let mut rule_names: Vec<_> = incompatible_rules
.into_iter()
.map(|rule| format!("`{}`", rule.noqa_code()))
.map(|rule| format!("{:#}", rule.name_and_code()))
.collect();
rule_names.sort();
if let [rule] = rule_names.as_slice() {
Expand Down
35 changes: 21 additions & 14 deletions crates/ruff/src/commands/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use crate::args::HelpFormat;
#[derive(Serialize)]
struct Explanation<'a> {
name: &'a str,
code: String,
linter: &'a str,
code: Option<String>,
linter: Option<&'a str>,
summary: &'a str,
message_formats: &'a [&'a str],
fix: String,
Expand All @@ -31,13 +31,16 @@ struct Explanation<'a> {

impl<'a> Explanation<'a> {
fn from_rule(rule: &'a Rule) -> Self {
let code = rule.noqa_code().to_string();
let (linter, _) = Linter::parse_code(&code).unwrap();
let code = rule.noqa_code().map(|code| code.to_string());
let linter = code
.as_deref()
.and_then(Linter::parse_code)
.map(|(linter, _)| linter.name());
let fix = rule.fixable().to_string();
Self {
name: rule.name().as_str(),
code,
linter: linter.name(),
linter,
summary: rule.message_formats()[0],
message_formats: rule.message_formats(),
fix,
Expand All @@ -56,18 +59,22 @@ impl<'a> Explanation<'a> {

fn format_rule_text(rule: Rule) -> String {
let mut output = String::new();
let _ = write!(&mut output, "# {} ({})", rule.name(), rule.noqa_code());
let _ = write!(&mut output, "# {}", rule.name_and_code());
output.push('\n');
output.push('\n');

let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap();
let _ = write!(
&mut output,
"Derived from the **{}** linter.",
linter.name()
);
output.push('\n');
output.push('\n');
if let Some(linter) = rule
.noqa_code()
.and_then(|code| Linter::parse_code(&code.to_string()).map(|(linter, _)| linter))
{
let _ = write!(
&mut output,
"Derived from the **{}** linter.",
linter.name()
);
output.push('\n');
output.push('\n');
}

let fix_availability = rule.fixable();
if matches!(
Expand Down
Comment thread
ntBre marked this conversation as resolved.
File renamed without changes.
4 changes: 2 additions & 2 deletions crates/ruff/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ impl AddAssign for FixMap {
continue;
}
let fixed_in_file = self.0.entry(filename).or_default();
for (rule, name, count) in fixed.iter() {
for (id, code, count) in fixed.iter() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we do in this commit seems the right approach to me. We use the name as identifier, and only carry code along because we need it to preserve existing display behavior.

if count > 0 {
*fixed_in_file.entry(rule).or_default(name) += count;
*fixed_in_file.entry(id).or_default(code) += count;
}
}
}
Expand Down
33 changes: 16 additions & 17 deletions crates/ruff/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl ExpandedStatistics<'_> {
}
}

/// Accumulator type for grouping diagnostics by code.
/// Accumulator type for grouping diagnostics by name.
/// Format: (`code`, `representative_diagnostic`, `total_count`, `fixable_count`)
type DiagnosticGroup<'a> = (Option<&'a SecondaryCode>, &'a Diagnostic, usize, usize);

Expand Down Expand Up @@ -280,15 +280,15 @@ impl Printer {
let statistics: Vec<ExpandedStatistics> = diagnostics
.inner
.iter()
.sorted_by_key(|diagnostic| diagnostic.secondary_code())
.sorted_by_key(|diagnostic| diagnostic.name())
.fold(vec![], |mut acc: Vec<DiagnosticGroup>, diagnostic| {
let is_fixable = diagnostic
.fix()
.is_some_and(|fix| fix.applies(required_applicability));
let code = diagnostic.secondary_code();

if let Some((prev_code, _prev_message, count, fixable_count)) = acc.last_mut() {
if *prev_code == code {
if let Some((_prev_code, prev_message, count, fixable_count)) = acc.last_mut() {
if prev_message.name() == diagnostic.name() {
*count += 1;
if is_fixable {
*fixable_count += 1;
Expand Down Expand Up @@ -500,19 +500,18 @@ fn print_fix_summary(
relativize_path(filename).bold(),
":".cyan()
)?;
for (code, name, count) in table.iter().sorted_by_key(|(.., count)| Reverse(*count)) {
if is_human_readable_names_enabled(preview) && !prefer_rule_codes {
writeln!(
writer,
" {count:>num_digits$} × {name} ({code})",
name = name.to_string().red().bold(),
)?;
} else {
writeln!(
writer,
" {count:>num_digits$} × {code} ({name})",
code = code.to_string().red().bold(),
)?;
for (name, code, count) in table.iter().sorted_by_key(|(.., count)| Reverse(*count)) {
write!(writer, " {count:>num_digits$} × ")?;
match code {
Some(code) if is_human_readable_names_enabled(preview) && !prefer_rule_codes => {
writeln!(writer, "{name} ({code})", name = name.as_str().red().bold())?;
}
Some(code) => {
writeln!(writer, "{code} ({name})", code = code.as_str().red().bold())?;
}
None => {
writeln!(writer, "{name}", name = name.as_str().red().bold())?;
}
}
Comment thread
MichaReiser marked this conversation as resolved.
}
}
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff/tests/cli/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ if condition:
print('Should change quotes')

----- stderr -----
warning: The following rule may cause conflicts when used with the formatter: `COM812`. To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The following rule may cause conflicts when used with the formatter: `missing-trailing-comma` (`COM812`). To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
"#);
Ok(())
}
Expand Down Expand Up @@ -1187,7 +1187,7 @@ def say_hy(name: str):
1 file reformatted

----- stderr -----
warning: The following rule may cause conflicts when used with the formatter: `COM812`. To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The following rule may cause conflicts when used with the formatter: `missing-trailing-comma` (`COM812`). To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The `format.indent-style="tab"` option is incompatible with `W191`, which lints against all uses of tabs. We recommend disabling these rules when using the formatter, which enforces a consistent indentation style. Alternatively, set the `format.indent-style` option to `"space"`.
warning: The `lint.flake8-implicit-str-concat.allow-multiline = false` option is incompatible with the formatter unless `ISC001` is enabled. We recommend enabling `ISC001` or setting `allow-multiline=true`.
warning: The `format.indent-style="tab"` option is incompatible with `D206`, with requires space-based indentation. We recommend disabling these rules when using the formatter, which enforces a consistent indentation style. Alternatively, set the `format.indent-style` option to `"space"`.
Expand Down Expand Up @@ -1245,7 +1245,7 @@ def say_hy(name: str):
print(f"Hy {name}")

----- stderr -----
warning: The following rule may cause conflicts when used with the formatter: `COM812`. To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The following rule may cause conflicts when used with the formatter: `missing-trailing-comma` (`COM812`). To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The `format.indent-style="tab"` option is incompatible with `W191`, which lints against all uses of tabs. We recommend disabling these rules when using the formatter, which enforces a consistent indentation style. Alternatively, set the `format.indent-style` option to `"space"`.
warning: The `format.indent-style="tab"` option is incompatible with `D206`, with requires space-based indentation. We recommend disabling these rules when using the formatter, which enforces a consistent indentation style. Alternatively, set the `format.indent-style` option to `"space"`.
warning: The `flake8-quotes.inline-quotes="single"` option is incompatible with the formatter's `format.quote-style="double"`. We recommend disabling `Q000` and `Q003` when using the formatter, which enforces a consistent quote style. Alternatively, set both options to either `"single"` or `"double"`.
Expand Down Expand Up @@ -1378,7 +1378,7 @@ def say_hy(name: str):
----- stderr -----
warning: `incorrect-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `incorrect-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
warning: The following rule may cause conflicts when used with the formatter: `COM812`. To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
warning: The following rule may cause conflicts when used with the formatter: `missing-trailing-comma` (`COM812`). To avoid unexpected behavior, we recommend disabling this rule, either by removing it from the `lint.select` or `lint.extend-select` configuration, or adding it to the `lint.ignore` configuration.
");
Ok(())
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_dev/src/generate_default_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub(crate) fn generate() -> String {
output.push_str(" select = [\n");
for (_, rules) in &linters {
for rule in rules {
let _ = writeln!(output, " \"{}\",", rule.noqa_code());
let _ = writeln!(output, " \"{}\",", rule.noqa_code().unwrap());
Comment thread
MichaReiser marked this conversation as resolved.
}
}
output.push_str(" ]\n");
Expand All @@ -56,7 +56,7 @@ pub(crate) fn generate() -> String {

for rule in rules {
let name = rule.name();
let code = rule.noqa_code();
let code = rule.noqa_code().unwrap();
let _ = writeln!(output, "- [`{name}`](rules/{name}.md) (`{code}`)");
}
output.push('\n');
Expand Down
32 changes: 19 additions & 13 deletions crates/ruff_dev/src/generate_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub(crate) fn main(args: &Args) -> Result<()> {
if let Some(explanation) = rule.explanation() {
let mut output = String::new();

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

let status_text = match rule.status() {
RuleStatus::Stable { since } => {
Expand All @@ -55,26 +55,36 @@ pub(crate) fn main(args: &Args) -> Result<()> {
}
};

let issue_search = format!(
"(%27{encoded_name}%27{code})",
encoded_name =
url::form_urlencoded::byte_serialize(rule.name().as_str().as_bytes())
.collect::<String>(),
code = rule
.noqa_code()
.map(|code| format!("%20OR%20{code}"))
.unwrap_or_default(),
);

let _ = writeln!(
&mut output,
r#"<small>
{status_text} ·
<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> ·
<a href="https://github.com/astral-sh/ruff/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20{issue_search}" target="_blank">Related issues</a> ·
<a href="https://github.com/astral-sh/ruff/blob/main/{file}#L{line}" target="_blank">View source</a>
</small>

"#,
encoded_name =
url::form_urlencoded::byte_serialize(rule.name().as_str().as_bytes())
.collect::<String>(),
rule_code = rule.noqa_code(),
file =
url::form_urlencoded::byte_serialize(rule.file().replace('\\', "/").as_bytes())
.collect::<String>(),
line = rule.line(),
);
let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap();
if linter.url().is_some() {
if let Some(linter) = rule
.noqa_code()
.and_then(|code| Linter::parse_code(&code.to_string()).map(|(linter, _)| linter))
.filter(|linter| linter.url().is_some())
{
let common_prefix: String = match linter.common_prefix() {
"" => linter
.upstream_categories()
Expand Down Expand Up @@ -134,11 +144,7 @@ pub(crate) fn main(args: &Args) -> Result<()> {
output.push('\n');
}

process_documentation(
explanation.trim(),
&mut output,
&rule.noqa_code().to_string(),
);
process_documentation(explanation.trim(), &mut output, rule.name().as_str());

let filename = PathBuf::from(ROOT_DIR)
.join("docs")
Expand Down
Loading