Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't enforce returns and yields in abstract methods #12771

Merged
merged 1 commit into from
Aug 9, 2024
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
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,14 @@ def f(num: int):
num (int): A number
"""
return 1


import abc


class A(metaclass=abc.abcmeta):
# DOC201
@abc.abstractmethod
def f(self):
"""Lorem ipsum."""
return True
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,14 @@ def baz(self) -> str:
A number
"""
return 'test'


import abc


class A(metaclass=abc.abcmeta):
# DOC201
@abc.abstractmethod
def f(self):
"""Lorem ipsum."""
return True
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,17 @@ def foo(self) -> int:
x
"""
raise NotImplementedError


import abc


class A(metaclass=abc.abcmeta):
@abc.abstractmethod
def f(self):
"""Lorem ipsum

Returns:
dict: The values
"""
return
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,19 @@ def bar(self) -> str:
A number
"""
print('test')


import abc


class A(metaclass=abc.abcmeta):
@abc.abstractmethod
def f(self):
"""Lorem ipsum

Returns
-------
dict:
The values
"""
return
112 changes: 64 additions & 48 deletions crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, visitor, Expr, Stmt};
use ruff_python_semantic::analyze::function_type;
use ruff_python_semantic::analyze::{function_type, visibility};
use ruff_python_semantic::{Definition, SemanticModel};
use ruff_text_size::{Ranged, TextRange};

Expand All @@ -25,6 +25,8 @@ use crate::rules::pydocstyle::settings::Convention;
/// Docstrings missing return sections are a sign of incomplete documentation
/// or refactors.
///
/// This rule is not enforced for abstract methods and stubs functions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This rule is not enforced for abstract methods and stubs functions.
/// This rule is not enforced for stub functions.

///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -73,6 +75,8 @@ impl Violation for DocstringMissingReturns {
/// Functions without an explicit return should not have a returns section
/// in their docstrings.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def say_hello(n: int) -> None:
Expand Down Expand Up @@ -121,6 +125,8 @@ impl Violation for DocstringExtraneousReturns {
/// Docstrings missing yields sections are a sign of incomplete documentation
/// or refactors.
///
/// This rule is not enforced for abstract methods and stubs functions.
///
/// ## Example
/// ```python
/// def count_to_n(n: int) -> int:
Expand Down Expand Up @@ -169,6 +175,8 @@ impl Violation for DocstringMissingYields {
/// Functions which don't yield anything should not have a yields section
/// in their docstrings.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def say_hello(n: int) -> None:
Expand Down Expand Up @@ -218,6 +226,8 @@ impl Violation for DocstringExtraneousYields {
/// it can be misleading to users and/or a sign of incomplete documentation or
/// refactors.
///
/// This rule is not enforced for abstract methods and stubs functions.
///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -282,6 +292,8 @@ impl Violation for DocstringMissingException {
/// Some conventions prefer non-explicit exceptions be omitted from the
/// docstring.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -343,7 +355,7 @@ impl Violation for DocstringExtraneousException {
}
}

// A generic docstring section.
/// A generic docstring section.
#[derive(Debug)]
struct GenericSection {
range: TextRange,
Expand All @@ -363,7 +375,7 @@ impl GenericSection {
}
}

// A Raises docstring section.
/// A "Raises" section in a docstring.
#[derive(Debug)]
struct RaisesSection<'a> {
raised_exceptions: Vec<QualifiedName<'a>>,
Expand All @@ -378,7 +390,7 @@ impl Ranged for RaisesSection<'_> {

impl<'a> RaisesSection<'a> {
/// Return the raised exceptions for the docstring, or `None` if the docstring does not contain
/// a `Raises` section.
/// a "Raises" section.
fn from_section(section: &SectionContext<'a>, style: Option<SectionStyle>) -> Self {
Self {
raised_exceptions: parse_entries(section.following_lines_str(), style),
Expand Down Expand Up @@ -415,7 +427,7 @@ impl<'a> DocstringSections<'a> {
}
}

/// Parse the entries in a `Raises` section of a docstring.
/// Parse the entries in a "Raises" section of a docstring.
///
/// Attempts to parse using the specified [`SectionStyle`], falling back to the other style if no
/// entries are found.
Expand Down Expand Up @@ -519,7 +531,7 @@ struct BodyEntries<'a> {
struct BodyVisitor<'a> {
returns: Vec<Entry>,
yields: Vec<Entry>,
currently_suspended_exceptions: Option<&'a ast::Expr>,
currently_suspended_exceptions: Option<&'a Expr>,
raised_exceptions: Vec<ExceptionEntry<'a>>,
semantic: &'a SemanticModel<'a>,
}
Expand Down Expand Up @@ -732,17 +744,6 @@ pub(crate) fn check_docstring(
}
}

// DOC202
if checker.enabled(Rule::DocstringExtraneousReturns) {
if let Some(ref docstring_returns) = docstring_sections.returns {
if body_entries.returns.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousReturns, docstring_returns.range());
diagnostics.push(diagnostic);
}
}
}

// DOC402
if checker.enabled(Rule::DocstringMissingYields) {
if !yields_documented(docstring, &docstring_sections, convention) {
Expand All @@ -753,17 +754,6 @@ pub(crate) fn check_docstring(
}
}

// DOC403
if checker.enabled(Rule::DocstringExtraneousYields) {
if let Some(docstring_yields) = docstring_sections.yields {
if body_entries.yields.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousYields, docstring_yields.range());
diagnostics.push(diagnostic);
}
}
}

// DOC501
if checker.enabled(Rule::DocstringMissingException) {
for body_raise in &body_entries.raised_exceptions {
Expand Down Expand Up @@ -794,28 +784,54 @@ pub(crate) fn check_docstring(
}
}

// DOC502
if checker.enabled(Rule::DocstringExtraneousException) {
if let Some(docstring_raises) = docstring_sections.raises {
let mut extraneous_exceptions = Vec::new();
for docstring_raise in &docstring_raises.raised_exceptions {
if !body_entries.raised_exceptions.iter().any(|exception| {
exception
.qualified_name
.segments()
.ends_with(docstring_raise.segments())
}) {
extraneous_exceptions.push(docstring_raise.to_string());
// Avoid applying "extraneous" rules to abstract methods. An abstract method's docstring _could_
// document that it raises an exception without including the exception in the implementation.
if !visibility::is_abstract(&function_def.decorator_list, checker.semantic()) {
// DOC202
if checker.enabled(Rule::DocstringExtraneousReturns) {
if let Some(ref docstring_returns) = docstring_sections.returns {
if body_entries.returns.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousReturns, docstring_returns.range());
diagnostics.push(diagnostic);
}
}
if !extraneous_exceptions.is_empty() {
let diagnostic = Diagnostic::new(
DocstringExtraneousException {
ids: extraneous_exceptions,
},
docstring_raises.range(),
);
diagnostics.push(diagnostic);
}

// DOC403
if checker.enabled(Rule::DocstringExtraneousYields) {
if let Some(docstring_yields) = docstring_sections.yields {
if body_entries.yields.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousYields, docstring_yields.range());
diagnostics.push(diagnostic);
}
}
}

// DOC502
if checker.enabled(Rule::DocstringExtraneousException) {
if let Some(docstring_raises) = docstring_sections.raises {
let mut extraneous_exceptions = Vec::new();
for docstring_raise in &docstring_raises.raised_exceptions {
if !body_entries.raised_exceptions.iter().any(|exception| {
exception
.qualified_name
.segments()
.ends_with(docstring_raise.segments())
}) {
extraneous_exceptions.push(docstring_raise.to_string());
}
}
if !extraneous_exceptions.is_empty() {
let diagnostic = Diagnostic::new(
DocstringExtraneousException {
ids: extraneous_exceptions,
},
docstring_raises.range(),
);
diagnostics.push(diagnostic);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,12 @@ DOC201_google.py:71:9: DOC201 `return` is not documented in docstring
73 | print("I never return")
|
= help: Add a "Returns" section to the docstring

DOC201_google.py:121:9: DOC201 `return` is not documented in docstring
|
119 | def f(self):
120 | """Lorem ipsum."""
121 | return True
| ^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,12 @@ DOC201_numpy.py:62:9: DOC201 `return` is not documented in docstring
| ^^^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring

DOC201_numpy.py:87:9: DOC201 `return` is not documented in docstring
|
85 | def f(self):
86 | """Lorem ipsum."""
87 | return True
| ^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring
Loading