diff --git a/.gitignore b/.gitignore index 8a817cec8e..1a2f8eb69f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,8 @@ # Local cache .ruff_cache crates/ruff/resources/test/cpython -docs/ +docs/* +!docs/rules mkdocs.yml .overrides diff --git a/README.md b/README.md index 4ffa75f12e..89632309cf 100644 --- a/README.md +++ b/README.md @@ -957,7 +957,7 @@ For more, see [flake8-bugbear](https://pypi.org/project/flake8-bugbear/) on PyPI | B014 | duplicate-handler-exception | Exception handler with duplicate exception: `{name}` | 🛠 | | B015 | useless-comparison | Pointless comparison. This comparison does nothing but waste CPU instructions. Either prepend `assert` or remove it. | | | B016 | cannot-raise-literal | Cannot raise a literal. Did you intend to return it or raise an Exception? | | -| B017 | no-assert-raises-exception | `assertRaises(Exception)` should be considered evil | | +| [B017](https://github.com/charliermarsh/ruff/blob/main/docs/rules/assert-raises-exception.md) | [assert-raises-exception](https://github.com/charliermarsh/ruff/blob/main/docs/rules/assert-raises-exception.md) | `assertRaises(Exception)` should be considered evil | | | B018 | useless-expression | Found useless expression. Either assign it to a variable or remove it. | | | B019 | cached-instance-method | Use of `functools.lru_cache` or `functools.cache` on methods can lead to memory leaks | | | B020 | loop-variable-overrides-iterator | Loop control variable `{name}` overrides iterable it iterates | | diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 7aa9996a46..e17ce42c36 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -1563,7 +1563,7 @@ where } } StmtKind::With { items, body, .. } => { - if self.settings.rules.enabled(&Rule::NoAssertRaisesException) { + if self.settings.rules.enabled(&Rule::AssertRaisesException) { flake8_bugbear::rules::assert_raises_exception(self, stmt, items); } if self diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index adf8dafb61..bdbb41ca2c 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -146,7 +146,7 @@ ruff_macros::define_rule_mapping!( B014 => rules::flake8_bugbear::rules::DuplicateHandlerException, B015 => rules::flake8_bugbear::rules::UselessComparison, B016 => rules::flake8_bugbear::rules::CannotRaiseLiteral, - B017 => rules::flake8_bugbear::rules::NoAssertRaisesException, + B017 => rules::flake8_bugbear::rules::AssertRaisesException, B018 => rules::flake8_bugbear::rules::UselessExpression, B019 => rules::flake8_bugbear::rules::CachedInstanceMethod, B020 => rules::flake8_bugbear::rules::LoopVariableOverridesIterator, diff --git a/crates/ruff/src/rules/flake8_bugbear/mod.rs b/crates/ruff/src/rules/flake8_bugbear/mod.rs index 8888a12b37..4d9a182d04 100644 --- a/crates/ruff/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff/src/rules/flake8_bugbear/mod.rs @@ -29,7 +29,7 @@ mod tests { #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"); "B014")] #[test_case(Rule::UselessComparison, Path::new("B015.py"); "B015")] #[test_case(Rule::CannotRaiseLiteral, Path::new("B016.py"); "B016")] - #[test_case(Rule::NoAssertRaisesException, Path::new("B017.py"); "B017")] + #[test_case(Rule::AssertRaisesException, Path::new("B017.py"); "B017")] #[test_case(Rule::UselessExpression, Path::new("B018.py"); "B018")] #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"); "B019")] #[test_case(Rule::LoopVariableOverridesIterator, Path::new("B020.py"); "B020")] diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs b/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs index c76b12bd96..ef0f109e56 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs @@ -10,15 +10,25 @@ define_violation!( /// ### What it does /// Checks for `self.assertRaises(Exception)`. /// - /// ## Why is this bad? + /// ### Why is this bad? /// `assertRaises(Exception)` can lead to your test passing even if the /// code being tested is never executed due to a typo. /// /// Either assert for a more specific exception (builtin or custom), use /// `assertRaisesRegex` or the context manager form of `assertRaises`. - pub struct NoAssertRaisesException; + /// + /// ### Example + /// ```python + /// self.assertRaises(Exception, foo) + /// ``` + /// + /// Use instead: + /// ```python + /// self.assertRaises(SomeSpecificException, foo) + /// ``` + pub struct AssertRaisesException; ); -impl Violation for NoAssertRaisesException { +impl Violation for AssertRaisesException { #[derive_message_formats] fn message(&self) -> String { format!("`assertRaises(Exception)` should be considered evil") @@ -51,7 +61,7 @@ pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[With } checker.diagnostics.push(Diagnostic::new( - NoAssertRaisesException, + AssertRaisesException, Range::from_located(stmt), )); } diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs index 4473619f3c..d59c8ea853 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs @@ -3,7 +3,7 @@ pub use abstract_base_class::{ EmptyMethodWithoutAbstractDecorator, }; pub use assert_false::{assert_false, DoNotAssertFalse}; -pub use assert_raises_exception::{assert_raises_exception, NoAssertRaisesException}; +pub use assert_raises_exception::{assert_raises_exception, AssertRaisesException}; pub use assignment_to_os_environ::{assignment_to_os_environ, AssignmentToOsEnviron}; pub use cached_instance_method::{cached_instance_method, CachedInstanceMethod}; pub use cannot_raise_literal::{cannot_raise_literal, CannotRaiseLiteral}; diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap index ba5bfecd2c..182e81c2c3 100644 --- a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap +++ b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap @@ -1,9 +1,9 @@ --- -source: src/rules/flake8_bugbear/mod.rs +source: crates/ruff/src/rules/flake8_bugbear/mod.rs expression: diagnostics --- - kind: - NoAssertRaisesException: ~ + AssertRaisesException: ~ location: row: 22 column: 8 diff --git a/crates/ruff_cli/src/commands.rs b/crates/ruff_cli/src/commands.rs index 44ff526213..77cfac3a28 100644 --- a/crates/ruff_cli/src/commands.rs +++ b/crates/ruff_cli/src/commands.rs @@ -278,25 +278,31 @@ pub fn rule(rule: &Rule, format: HelpFormat) -> Result<()> { let mut stdout = BufWriter::new(io::stdout().lock()); match format { HelpFormat::Text => { - writeln!(stdout, "{}\n", rule.as_ref())?; - writeln!(stdout, "Code: {} ({})\n", rule.code(), linter.name())?; - + writeln!( + stdout, + "[{}] {} ({})", + linter.name(), + rule.as_ref(), + rule.code(), + )?; + writeln!(stdout)?; if let Some(explanation) = rule.explanation() { - writeln!(stdout, "{}\n", explanation)?; + writeln!(stdout, "{}", explanation.trim())?; } else { - writeln!(stdout, "Message formats:\n")?; + writeln!(stdout, "Message formats:")?; for format in rule.message_formats() { writeln!(stdout, "* {format}")?; } } if let Some(autofix) = rule.autofixable() { + writeln!(stdout)?; writeln!( stdout, "{}", match autofix.available { - AutofixAvailability::Sometimes => "Autofix is sometimes available.\n", - AutofixAvailability::Always => "Autofix is always available.\n", + AutofixAvailability::Sometimes => "Autofix is sometimes available.", + AutofixAvailability::Always => "Autofix is always available.", } )?; } diff --git a/crates/ruff_dev/src/generate_all.rs b/crates/ruff_dev/src/generate_all.rs index bba15fb2c2..71a63ed613 100644 --- a/crates/ruff_dev/src/generate_all.rs +++ b/crates/ruff_dev/src/generate_all.rs @@ -2,7 +2,9 @@ use anyhow::Result; -use crate::{generate_cli_help, generate_json_schema, generate_options, generate_rules_table}; +use crate::{ + generate_cli_help, generate_docs, generate_json_schema, generate_options, generate_rules_table, +}; #[derive(clap::Args)] pub struct Args { @@ -12,6 +14,9 @@ pub struct Args { } pub fn main(args: &Args) -> Result<()> { + generate_docs::main(&generate_docs::Args { + dry_run: args.dry_run, + })?; generate_json_schema::main(&generate_json_schema::Args { dry_run: args.dry_run, })?; diff --git a/crates/ruff_dev/src/generate_docs.rs b/crates/ruff_dev/src/generate_docs.rs index 8b13789179..5ec52a92b1 100644 --- a/crates/ruff_dev/src/generate_docs.rs +++ b/crates/ruff_dev/src/generate_docs.rs @@ -1 +1,32 @@ +//! Generate Markdown documentation for applicable rules. +#![allow(clippy::print_stdout, clippy::print_stderr)] +use std::fs; + +use anyhow::Result; +use strum::IntoEnumIterator; + +use ruff::registry::Rule; + +#[derive(clap::Args)] +pub struct Args { + /// Write the generated docs to stdout (rather than to the filesystem). + #[arg(long)] + pub(crate) dry_run: bool, +} + +pub fn main(args: &Args) -> Result<()> { + for rule in Rule::iter() { + if let Some(explanation) = rule.explanation() { + let explanation = format!("# {} ({})\n\n{}", rule.as_ref(), rule.code(), explanation); + + if args.dry_run { + println!("{}", explanation); + } else { + fs::create_dir_all("docs/rules")?; + fs::write(format!("docs/rules/{}.md", rule.as_ref()), explanation)?; + } + } + } + Ok(()) +} diff --git a/crates/ruff_dev/src/generate_rules_table.rs b/crates/ruff_dev/src/generate_rules_table.rs index bce90da1ea..8a6f215fbd 100644 --- a/crates/ruff_dev/src/generate_rules_table.rs +++ b/crates/ruff_dev/src/generate_rules_table.rs @@ -14,6 +14,8 @@ const TABLE_END_PRAGMA: &str = ""; const TOC_BEGIN_PRAGMA: &str = ""; const TOC_END_PRAGMA: &str = ""; +const URL_PREFIX: &str = "https://github.com/charliermarsh/ruff/blob/main/docs/rules"; + #[derive(clap::Args)] pub struct Args { /// Write the generated table to stdout (rather than to `README.md`). @@ -32,13 +34,27 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator) Some(_) => "🛠", }; - table_out.push_str(&format!( - "| {} | {} | {} | {} |", - rule.code(), - rule.as_ref(), - rule.message_formats()[0].replace('|', r"\|"), - fix_token - )); + if rule.explanation().is_some() { + table_out.push_str(&format!( + "| [{}]({}/{}.md) | [{}]({}/{}.md) | {} | {} |", + rule.code(), + URL_PREFIX, + rule.as_ref(), + rule.as_ref(), + URL_PREFIX, + rule.as_ref(), + rule.message_formats()[0].replace('|', r"\|"), + fix_token + )); + } else { + table_out.push_str(&format!( + "| {} | {} | {} | {} |", + rule.code(), + rule.as_ref(), + rule.message_formats()[0].replace('|', r"\|"), + fix_token + )); + } table_out.push('\n'); } table_out.push('\n'); diff --git a/crates/ruff_dev/src/main.rs b/crates/ruff_dev/src/main.rs index 13916784ef..42af270726 100644 --- a/crates/ruff_dev/src/main.rs +++ b/crates/ruff_dev/src/main.rs @@ -39,6 +39,8 @@ enum Command { GenerateOptions(generate_options::Args), /// Generate CLI help. GenerateCliHelp(generate_cli_help::Args), + /// Generate Markdown docs. + GenerateDocs(generate_docs::Args), /// Print the AST for a given Python file. PrintAST(print_ast::Args), /// Print the LibCST CST for a given Python file. @@ -57,6 +59,7 @@ fn main() -> Result<()> { Command::GenerateRulesTable(args) => generate_rules_table::main(args)?, Command::GenerateOptions(args) => generate_options::main(args)?, Command::GenerateCliHelp(args) => generate_cli_help::main(args)?, + Command::GenerateDocs(args) => generate_docs::main(args)?, Command::PrintAST(args) => print_ast::main(args)?, Command::PrintCST(args) => print_cst::main(args)?, Command::PrintTokens(args) => print_tokens::main(args)?, diff --git a/crates/ruff_macros/src/define_violation.rs b/crates/ruff_macros/src/define_violation.rs index 9c9b0a7fd3..1d8e307d90 100644 --- a/crates/ruff_macros/src/define_violation.rs +++ b/crates/ruff_macros/src/define_violation.rs @@ -37,7 +37,8 @@ impl Parse for LintMeta { let value = lit.value(); let line = value.strip_prefix(' ').unwrap_or(&value); if line.starts_with("```") { - explanation += "```\n"; + explanation += line; + explanation.push('\n'); in_code = !in_code; } else if !(in_code && line.starts_with("# ")) { explanation += line; @@ -61,15 +62,21 @@ impl Parse for LintMeta { pub fn define_violation(input: &TokenStream, meta: LintMeta) -> TokenStream { let LintMeta { explanation, name } = meta; - let output = quote! { - #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] - #input + if explanation.is_empty() { + quote! { + #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] + #input + } + } else { + quote! { + #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] + #input - impl #name { - pub fn explanation() -> Option<&'static str> { - Some(#explanation) + impl #name { + pub fn explanation() -> Option<&'static str> { + Some(#explanation) + } } } - }; - output + } } diff --git a/docs/rules/assert-raises-exception.md b/docs/rules/assert-raises-exception.md new file mode 100644 index 0000000000..fb8d0074bd --- /dev/null +++ b/docs/rules/assert-raises-exception.md @@ -0,0 +1,21 @@ +# assert-raises-exception (B017) + +### What it does +Checks for the use of `assertRaises(Exception)`. + +### Why is this bad? +`assertRaises(Exception)` can lead to your test passing even if the +code being tested is never executed (e.g., due to a typo). + +Assert for a more specific exception (builtin or custom), use +`assertRaisesRegex` or the context manager form of `assertRaises`. + +### Example +```python +self.assertRaises(Exception, foo) +``` + +Use instead: +```python +self.assertRaises(SomeSpecificException, foo) +``` diff --git a/scripts/add_rule.py b/scripts/add_rule.py index c3f93803a9..f64676d3ca 100755 --- a/scripts/add_rule.py +++ b/scripts/add_rule.py @@ -21,10 +21,15 @@ def snake_case(name: str) -> str: ).lstrip("_") -def main(*, name: str, code: str, linter: str) -> None: # noqa: PLR0915 +def main(*, name: str, code: str, linter: str) -> None: """Generate boilerplate for a new rule.""" # Create a test fixture. - with (ROOT_DIR / "crates/ruff/resources/test/fixtures" / dir_name(linter) / f"{code}.py").open( + with ( + ROOT_DIR + / "crates/ruff/resources/test/fixtures" + / dir_name(linter) + / f"{code}.py" + ).open( "a", ): pass diff --git a/scripts/generate_mkdocs.py b/scripts/generate_mkdocs.py index b100b0dcd3..b2bba42d74 100644 --- a/scripts/generate_mkdocs.py +++ b/scripts/generate_mkdocs.py @@ -36,6 +36,12 @@ def main() -> None: raise ValueError(msg) content = content.replace(DOCUMENTATION_LINK, "") + # Replace all GitHub links with relative links. + content = content.replace( + "https://github.com/charliermarsh/ruff/blob/main/docs/rules/", + "rules/", + ) + Path("docs").mkdir(parents=True, exist_ok=True) # Split the README.md into sections. @@ -71,8 +77,13 @@ def main() -> None: ] config["extra"] = {"analytics": {"provider": "fathom"}} - Path(".overrides/partials/integrations/analytics").mkdir(parents=True, exist_ok=True) - with Path(".overrides/partials/integrations/analytics/fathom.html").open("w+") as fp: + Path(".overrides/partials/integrations/analytics").mkdir( + parents=True, + exist_ok=True, + ) + with Path(".overrides/partials/integrations/analytics/fathom.html").open( + "w+", + ) as fp: fp.write(FATHOM_SCRIPT) with Path("mkdocs.yml").open("w+") as fp: