From 77055faab60d51478b2650d11335736ba7513576 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 12 Oct 2022 13:20:55 -0400 Subject: [PATCH] Implement D404 and D418 for pydocstyle (#409) --- README.md | 4 +- src/check_ast.rs | 6 ++ src/checks.rs | 90 +++++++++++--------- src/docstrings.rs | 45 ++++++++++ src/linter.rs | 24 ++++++ src/snapshots/ruff__linter__tests__d404.snap | 6 ++ src/snapshots/ruff__linter__tests__d418.snap | 29 +++++++ src/visibility.rs | 13 ++- 8 files changed, 176 insertions(+), 41 deletions(-) create mode 100644 src/snapshots/ruff__linter__tests__d404.snap create mode 100644 src/snapshots/ruff__linter__tests__d418.snap diff --git a/README.md b/README.md index f5c8f9c17b..b3bb2933f4 100644 --- a/README.md +++ b/README.md @@ -217,7 +217,7 @@ ruff also implements some of the most popular Flake8 plugins natively, including - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) (12/16) - [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (3/32) -- [`flake8-docstrings`](https://pypi.org/project/flake8-docstrings/) (25/48) +- [`flake8-docstrings`](https://pypi.org/project/flake8-docstrings/) (27/48) - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (8/34) Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8: @@ -323,7 +323,9 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com | D400 | EndsInPeriod | First line should end with a period | | | | D402 | NoSignature | First line should not be the function's 'signature' | | | | D403 | FirstLineCapitalized | First word of the first line should be properly capitalized | | | +| D404 | NoThisPrefix | First word of the docstring should not be `This` | | | | D415 | EndsInPunctuation | First line should end with a period, question mark, or exclamation point | | | +| D418 | SkipDocstring | Function decorated with @overload shouldn't contain a docstring | | | | D419 | NonEmpty | Docstring is empty | | | | D201 | NoBlankLineBeforeFunction | No blank lines allowed before function docstring (found 1) | | | | D202 | NoBlankLineAfterFunction | No blank lines allowed after function docstring (found 1) | | | diff --git a/src/check_ast.rs b/src/check_ast.rs index 76f2b1f42c..b9e44a60d0 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1955,9 +1955,15 @@ impl<'a> Checker<'a> { if self.settings.enabled.contains(&CheckCode::D403) { docstrings::capitalized(self, &docstring); } + if self.settings.enabled.contains(&CheckCode::D404) { + docstrings::starts_with_this(self, &docstring); + } if self.settings.enabled.contains(&CheckCode::D415) { docstrings::ends_with_punctuation(self, &docstring); } + if self.settings.enabled.contains(&CheckCode::D418) { + docstrings::if_needed(self, &docstring); + } } } diff --git a/src/checks.rs b/src/checks.rs index 086fc719f1..c434556759 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -170,7 +170,9 @@ pub enum CheckCode { D400, D402, D403, + D404, D415, + D418, D419, D201, D202, @@ -279,27 +281,29 @@ pub enum CheckKind { EndsInPunctuation, FirstLineCapitalized, FitsOnOneLine, + MagicMethod, MultiLineSummaryFirstLine, MultiLineSummarySecondLine, NewLineAfterLastParagraph, - NoBlankLineAfterSummary, - NoSurroundingWhitespace, - NonEmpty, - UsesTripleQuotes, - NoSignature, - NoBlankLineBeforeFunction(usize), NoBlankLineAfterFunction(usize), + NoBlankLineAfterSummary, NoBlankLineBeforeClass(usize), - OneBlankLineBeforeClass(usize), + NoBlankLineBeforeFunction(usize), + NoSignature, + NoSurroundingWhitespace, + NoThisPrefix, + NonEmpty, OneBlankLineAfterClass(usize), - PublicModule, + OneBlankLineBeforeClass(usize), PublicClass, - PublicMethod, PublicFunction, - PublicPackage, - MagicMethod, - PublicNestedClass, PublicInit, + PublicMethod, + PublicModule, + PublicNestedClass, + PublicPackage, + SkipDocstring, + UsesTripleQuotes, // Meta UnusedNOQA(Option>), } @@ -422,22 +426,24 @@ impl CheckCode { CheckCode::D106 => CheckKind::PublicNestedClass, CheckCode::D107 => CheckKind::PublicInit, CheckCode::D200 => CheckKind::FitsOnOneLine, + CheckCode::D201 => CheckKind::NoBlankLineBeforeFunction(1), + CheckCode::D202 => CheckKind::NoBlankLineAfterFunction(1), + CheckCode::D203 => CheckKind::OneBlankLineBeforeClass(0), + CheckCode::D204 => CheckKind::OneBlankLineAfterClass(0), CheckCode::D205 => CheckKind::NoBlankLineAfterSummary, CheckCode::D209 => CheckKind::NewLineAfterLastParagraph, CheckCode::D210 => CheckKind::NoSurroundingWhitespace, - CheckCode::D400 => CheckKind::EndsInPeriod, - CheckCode::D419 => CheckKind::NonEmpty, + CheckCode::D211 => CheckKind::NoBlankLineBeforeClass(1), CheckCode::D212 => CheckKind::MultiLineSummaryFirstLine, CheckCode::D213 => CheckKind::MultiLineSummarySecondLine, CheckCode::D300 => CheckKind::UsesTripleQuotes, + CheckCode::D400 => CheckKind::EndsInPeriod, CheckCode::D402 => CheckKind::NoSignature, CheckCode::D403 => CheckKind::FirstLineCapitalized, + CheckCode::D404 => CheckKind::NoThisPrefix, CheckCode::D415 => CheckKind::EndsInPunctuation, - CheckCode::D201 => CheckKind::NoBlankLineBeforeFunction(1), - CheckCode::D202 => CheckKind::NoBlankLineAfterFunction(1), - CheckCode::D211 => CheckKind::NoBlankLineBeforeClass(1), - CheckCode::D203 => CheckKind::OneBlankLineBeforeClass(0), - CheckCode::D204 => CheckKind::OneBlankLineAfterClass(0), + CheckCode::D418 => CheckKind::SkipDocstring, + CheckCode::D419 => CheckKind::NonEmpty, // Meta CheckCode::M001 => CheckKind::UnusedNOQA(None), } @@ -527,31 +533,33 @@ impl CheckKind { CheckKind::UselessObjectInheritance(_) => &CheckCode::U004, CheckKind::SuperCallWithParameters => &CheckCode::U008, // pydocstyle - CheckKind::PublicModule => &CheckCode::D100, - CheckKind::PublicClass => &CheckCode::D101, - CheckKind::PublicMethod => &CheckCode::D102, - CheckKind::PublicFunction => &CheckCode::D103, - CheckKind::PublicPackage => &CheckCode::D104, - CheckKind::MagicMethod => &CheckCode::D105, - CheckKind::PublicNestedClass => &CheckCode::D106, - CheckKind::PublicInit => &CheckCode::D107, - CheckKind::FitsOnOneLine => &CheckCode::D200, - CheckKind::NoBlankLineAfterSummary => &CheckCode::D205, - CheckKind::NewLineAfterLastParagraph => &CheckCode::D209, - CheckKind::NoSurroundingWhitespace => &CheckCode::D210, CheckKind::EndsInPeriod => &CheckCode::D400, - CheckKind::NonEmpty => &CheckCode::D419, + CheckKind::EndsInPunctuation => &CheckCode::D415, + CheckKind::FirstLineCapitalized => &CheckCode::D403, + CheckKind::FitsOnOneLine => &CheckCode::D200, + CheckKind::MagicMethod => &CheckCode::D105, CheckKind::MultiLineSummaryFirstLine => &CheckCode::D212, CheckKind::MultiLineSummarySecondLine => &CheckCode::D213, - CheckKind::UsesTripleQuotes => &CheckCode::D300, - CheckKind::NoSignature => &CheckCode::D402, - CheckKind::FirstLineCapitalized => &CheckCode::D403, - CheckKind::EndsInPunctuation => &CheckCode::D415, - CheckKind::NoBlankLineBeforeFunction(_) => &CheckCode::D201, + CheckKind::NewLineAfterLastParagraph => &CheckCode::D209, CheckKind::NoBlankLineAfterFunction(_) => &CheckCode::D202, + CheckKind::NoBlankLineAfterSummary => &CheckCode::D205, CheckKind::NoBlankLineBeforeClass(_) => &CheckCode::D211, - CheckKind::OneBlankLineBeforeClass(_) => &CheckCode::D203, + CheckKind::NoBlankLineBeforeFunction(_) => &CheckCode::D201, + CheckKind::NoSignature => &CheckCode::D402, + CheckKind::NoSurroundingWhitespace => &CheckCode::D210, + CheckKind::NoThisPrefix => &CheckCode::D404, + CheckKind::NonEmpty => &CheckCode::D419, CheckKind::OneBlankLineAfterClass(_) => &CheckCode::D204, + CheckKind::OneBlankLineBeforeClass(_) => &CheckCode::D203, + CheckKind::PublicClass => &CheckCode::D101, + CheckKind::PublicFunction => &CheckCode::D103, + CheckKind::PublicInit => &CheckCode::D107, + CheckKind::PublicMethod => &CheckCode::D102, + CheckKind::PublicModule => &CheckCode::D100, + CheckKind::PublicNestedClass => &CheckCode::D106, + CheckKind::PublicPackage => &CheckCode::D104, + CheckKind::SkipDocstring => &CheckCode::D418, + CheckKind::UsesTripleQuotes => &CheckCode::D300, // Meta CheckKind::UnusedNOQA(_) => &CheckCode::M001, } @@ -851,6 +859,12 @@ impl CheckKind { CheckKind::MagicMethod => "Missing docstring in magic method".to_string(), CheckKind::PublicNestedClass => "Missing docstring in public nested class".to_string(), CheckKind::PublicInit => "Missing docstring in __init__".to_string(), + CheckKind::NoThisPrefix => { + "First word of the docstring should not be `This`".to_string() + } + CheckKind::SkipDocstring => { + "Function decorated with @overload shouldn't contain a docstring".to_string() + } // Meta CheckKind::UnusedNOQA(codes) => match codes { None => "Unused `noqa` directive".to_string(), diff --git a/src/docstrings.rs b/src/docstrings.rs index 8118e335ea..efbefc8b1d 100644 --- a/src/docstrings.rs +++ b/src/docstrings.rs @@ -1,3 +1,5 @@ +//! Abstractions for tracking and validationg docstrings in Python code. + use once_cell::sync::Lazy; use regex::Regex; use rustpython_ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind}; @@ -593,6 +595,32 @@ pub fn capitalized(checker: &mut Checker, definition: &Definition) { } } +/// D404 +pub fn starts_with_this(checker: &mut Checker, definition: &Definition) { + if let Some(docstring) = definition.docstring { + if let ExprKind::Constant { + value: Constant::Str(string), + .. + } = &docstring.node + { + let trimmed = string.trim(); + if trimmed.is_empty() { + return; + } + + if let Some(first_word) = string.split(' ').next() { + if first_word + .replace(|c: char| !c.is_alphanumeric(), "") + .to_lowercase() + == "this" + { + checker.add_check(Check::new(CheckKind::NoThisPrefix, range_for(docstring))); + } + } + } + } +} + /// D415 pub fn ends_with_punctuation(checker: &mut Checker, definition: &Definition) { if let Some(docstring) = definition.docstring { @@ -613,6 +641,23 @@ pub fn ends_with_punctuation(checker: &mut Checker, definition: &Definition) { } } +/// D418 +pub fn if_needed(checker: &mut Checker, definition: &Definition) { + if definition.docstring.is_some() { + if let DefinitionKind::Function(stmt) + | DefinitionKind::NestedFunction(stmt) + | DefinitionKind::Method(stmt) = definition.kind + { + if is_overload(stmt) { + checker.add_check(Check::new( + CheckKind::SkipDocstring, + Range::from_located(stmt), + )); + } + } + } +} + /// D419 pub fn not_empty(checker: &mut Checker, definition: &Definition) -> bool { if let Some(docstring) = definition.docstring { diff --git a/src/linter.rs b/src/linter.rs index 95b45e2d7a..6b3fbc1a92 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -1284,6 +1284,18 @@ mod tests { Ok(()) } + #[test] + fn d404() -> Result<()> { + let mut checks = check_path( + Path::new("./resources/test/fixtures/D.py"), + &settings::Settings::for_rule(CheckCode::D404), + &fixer::Mode::Generate, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } + #[test] fn d415() -> Result<()> { let mut checks = check_path( @@ -1296,6 +1308,18 @@ mod tests { Ok(()) } + #[test] + fn d418() -> Result<()> { + let mut checks = check_path( + Path::new("./resources/test/fixtures/D.py"), + &settings::Settings::for_rule(CheckCode::D418), + &fixer::Mode::Generate, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } + #[test] fn d419() -> Result<()> { let mut checks = check_path( diff --git a/src/snapshots/ruff__linter__tests__d404.snap b/src/snapshots/ruff__linter__tests__d404.snap new file mode 100644 index 0000000000..60c615f917 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__d404.snap @@ -0,0 +1,6 @@ +--- +source: src/linter.rs +expression: checks +--- +[] + diff --git a/src/snapshots/ruff__linter__tests__d418.snap b/src/snapshots/ruff__linter__tests__d418.snap new file mode 100644 index 0000000000..65485ba315 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__d418.snap @@ -0,0 +1,29 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: SkipDocstring + location: + row: 33 + column: 5 + end_location: + row: 37 + column: 5 + fix: ~ +- kind: SkipDocstring + location: + row: 85 + column: 5 + end_location: + row: 89 + column: 5 + fix: ~ +- kind: SkipDocstring + location: + row: 105 + column: 1 + end_location: + row: 110 + column: 1 + fix: ~ + diff --git a/src/visibility.rs b/src/visibility.rs index 4587abec8f..9bd2d5c376 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -1,3 +1,5 @@ +//! Abstractions for tracking public and private visibility across modules, classes, and functions. + use std::path::Path; use crate::ast::helpers::match_name_or_attr; @@ -24,6 +26,7 @@ pub struct VisibleScope { pub visibility: Visibility, } +/// Returns `true` if a function definition is an `@overload`. pub fn is_overload(stmt: &Stmt) -> bool { match &stmt.node { StmtKind::FunctionDef { decorator_list, .. } @@ -34,6 +37,7 @@ pub fn is_overload(stmt: &Stmt) -> bool { } } +/// Returns `true` if a function is a "magic method". pub fn is_magic(stmt: &Stmt) -> bool { match &stmt.node { StmtKind::FunctionDef { name, .. } | StmtKind::AsyncFunctionDef { name, .. } => { @@ -47,6 +51,7 @@ pub fn is_magic(stmt: &Stmt) -> bool { } } +/// Returns `true` if a function is an `__init__`. pub fn is_init(stmt: &Stmt) -> bool { match &stmt.node { StmtKind::FunctionDef { name, .. } | StmtKind::AsyncFunctionDef { name, .. } => { @@ -56,13 +61,14 @@ pub fn is_init(stmt: &Stmt) -> bool { } } -fn is_private_name(module_name: &str) -> bool { +/// Returns `true` if a module name indicates private visibility. +fn is_private_module(module_name: &str) -> bool { module_name.starts_with('_') || (module_name.starts_with("__") && module_name.ends_with("__")) } pub fn module_visibility(path: &Path) -> Visibility { for component in path.iter().rev() { - if is_private_name(&component.to_string_lossy()) { + if is_private_module(&component.to_string_lossy()) { return Visibility::Private; } } @@ -115,6 +121,9 @@ fn class_visibility(stmt: &Stmt) -> Visibility { } /// Transition a `VisibleScope` based on a new `Documentable` definition. +/// +/// `scope` is the current `VisibleScope`, while `Documentable` and `Stmt` describe the current +/// node used to modify visibility. pub fn transition_scope(scope: &VisibleScope, stmt: &Stmt, kind: &Documentable) -> VisibleScope { match kind { Documentable::Function => VisibleScope {