From 6a6a79256230b6ac443e7f94fe97d163af756445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Sun, 22 Jan 2023 21:24:59 +0200 Subject: [PATCH] fix: issue D401 only for non-test/property functions and methods (#2071) Extend test fixture to verify the targeting. Includes two "attribute docstrings" which per PEP 257 are not recognized by the Python bytecode compiler or available as runtime object attributes. They are not available for us either at time of writing, but include them for completeness anyway in case they one day are. --- resources/test/fixtures/pydocstyle/D401.py | 60 ++++++++++++++++++- .../pydocstyle/rules/non_imperative_mood.rs | 15 ++++- ...ules__pydocstyle__tests__D401_D401.py.snap | 40 +++++++++---- src/visibility.rs | 14 +++++ 4 files changed, 117 insertions(+), 12 deletions(-) diff --git a/resources/test/fixtures/pydocstyle/D401.py b/resources/test/fixtures/pydocstyle/D401.py index fd98e9b1215f4..ae9b5843b2cba 100644 --- a/resources/test/fixtures/pydocstyle/D401.py +++ b/resources/test/fixtures/pydocstyle/D401.py @@ -1,3 +1,7 @@ +"""This module docstring does not need to be written in imperative mood.""" + +from functools import cached_property + # Bad examples def bad_liouiwnlkjl(): @@ -18,7 +22,11 @@ def bad_sdgfsdg23245777(): def bad_run_something(): """Runs something""" - pass + + def bad_nested(): + """Runs other things, nested""" + + bad_nested() def multi_line(): @@ -32,6 +40,11 @@ def multi_line(): def good_run_something(): """Run away.""" + def good_nested(): + """Run to the hills.""" + + good_nested() + def good_construct(): """Construct a beautiful house.""" @@ -41,3 +54,48 @@ def good_multi_line(): """Write a logical line that extends to two physical lines. """ + + +good_top_level_var = False +"""This top level assignment attribute docstring does not need to be written in imperative mood.""" + + +# Classes and their members + +class Thingy: + """This class docstring does not need to be written in imperative mood.""" + + _beep = "boop" + """This class attribute docstring does not need to be written in imperative mood.""" + + def bad_method(self): + """This method docstring should be written in imperative mood.""" + + @property + def good_property(self): + """This property method docstring does not need to be written in imperative mood.""" + return self._beep + + @cached_property + def good_cached_property(self): + """This property method docstring does not need to be written in imperative mood.""" + return 42 * 42 + + class NestedThingy: + """This nested class docstring does not need to be written in imperative mood.""" + + +# Test functions + +def test_something(): + """This test function does not need to be written in imperative mood. + + pydocstyle's rationale: + We exclude tests from the imperative mood check, because to phrase + their docstring in the imperative mood, they would have to start with + a highly redundant "Test that ..." + """ + + +def runTest(): + """This test function does not need to be written in imperative mood, either.""" diff --git a/src/rules/pydocstyle/rules/non_imperative_mood.rs b/src/rules/pydocstyle/rules/non_imperative_mood.rs index a01b60a392d59..50b588a28b848 100644 --- a/src/rules/pydocstyle/rules/non_imperative_mood.rs +++ b/src/rules/pydocstyle/rules/non_imperative_mood.rs @@ -2,18 +2,31 @@ use imperative::Mood; use once_cell::sync::Lazy; use ruff_macros::derive_message_formats; +use crate::ast::cast; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::define_violation; -use crate::docstrings::definition::Docstring; +use crate::docstrings::definition::{DefinitionKind, Docstring}; use crate::registry::Diagnostic; use crate::rules::pydocstyle::helpers::normalize_word; use crate::violation::Violation; +use crate::visibility::{is_property, is_test}; static MOOD: Lazy = Lazy::new(Mood::new); /// D401 pub fn non_imperative_mood(checker: &mut Checker, docstring: &Docstring) { + let ( + DefinitionKind::Function(parent) + | DefinitionKind::NestedFunction(parent) + | DefinitionKind::Method(parent) + ) = &docstring.kind else { + return; + }; + if is_test(cast::name(parent)) || is_property(checker, cast::decorator_list(parent)) { + return; + } + let body = docstring.body; // Find first line, disregarding whitespace. diff --git a/src/rules/pydocstyle/snapshots/ruff__rules__pydocstyle__tests__D401_D401.py.snap b/src/rules/pydocstyle/snapshots/ruff__rules__pydocstyle__tests__D401_D401.py.snap index 515431dd0d213..31acc02ec0692 100644 --- a/src/rules/pydocstyle/snapshots/ruff__rules__pydocstyle__tests__D401_D401.py.snap +++ b/src/rules/pydocstyle/snapshots/ruff__rules__pydocstyle__tests__D401_D401.py.snap @@ -5,51 +5,71 @@ expression: diagnostics - kind: NonImperativeMood: Returns foo. location: - row: 4 + row: 8 column: 4 end_location: - row: 4 + row: 8 column: 22 fix: ~ parent: ~ - kind: NonImperativeMood: Constructor for a foo. location: - row: 8 + row: 12 column: 4 end_location: - row: 8 + row: 12 column: 32 fix: ~ parent: ~ - kind: NonImperativeMood: Constructor for a boa. location: - row: 12 + row: 16 column: 4 end_location: - row: 16 + row: 20 column: 7 fix: ~ parent: ~ - kind: NonImperativeMood: Runs something location: - row: 20 + row: 24 column: 4 end_location: - row: 20 + row: 24 column: 24 fix: ~ parent: ~ +- kind: + NonImperativeMood: "Runs other things, nested" + location: + row: 27 + column: 8 + end_location: + row: 27 + column: 39 + fix: ~ + parent: ~ - kind: NonImperativeMood: Writes a logical line that location: - row: 25 + row: 33 column: 4 end_location: - row: 27 + row: 35 column: 7 fix: ~ parent: ~ +- kind: + NonImperativeMood: This method docstring should be written in imperative mood. + location: + row: 72 + column: 8 + end_location: + row: 72 + column: 73 + fix: ~ + parent: ~ diff --git a/src/visibility.rs b/src/visibility.rs index a49d5300eae6f..23423b175471a 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -70,6 +70,15 @@ pub fn is_abstract(checker: &Checker, decorator_list: &[Expr]) -> bool { }) } +/// Returns `true` if a function definition is a `@property`. +pub fn is_property(checker: &Checker, decorator_list: &[Expr]) -> bool { + decorator_list.iter().any(|expr| { + checker.resolve_call_path(expr).map_or(false, |call_path| { + call_path.as_slice() == ["", "property"] + || call_path.as_slice() == ["functools", "cached_property"] + }) + }) +} /// Returns `true` if a function is a "magic method". pub fn is_magic(name: &str) -> bool { name.starts_with("__") && name.ends_with("__") @@ -90,6 +99,11 @@ pub fn is_call(name: &str) -> bool { name == "__call__" } +/// Returns `true` if a function is a test one. +pub fn is_test(name: &str) -> bool { + name == "runTest" || name.starts_with("test") +} + /// Returns `true` if a module name indicates public visibility. fn is_public_module(module_name: &str) -> bool { !module_name.starts_with('_') || (module_name.starts_with("__") && module_name.ends_with("__"))