Skip to content

Commit

Permalink
[pydocstyle] Handle arguments with the same names as sections (`D41…
Browse files Browse the repository at this point in the history
…7`) (#16011)

## Summary

Fixes #16007. The logic from the last fix for this (#9427) was
sufficient, it just wasn't being applied because `Attributes` sections
aren't expected to have nested sections. I just deleted the outer
conditional, which should hopefully fix this for all section types.

## Test Plan

New regression test, plus the existing D417 tests.
  • Loading branch information
ntBre authored Feb 11, 2025
1 parent df1d430 commit 7b487d8
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 78 deletions.
43 changes: 42 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,45 @@ def f(x, *args, **kwargs):
x: the value
*args: var-arguments
"""
return x
return x


# regression test for https://github.com/astral-sh/ruff/issues/16007.
# attributes is a section name without subsections, so it was failing the
# previous workaround for Args: args: sections
def send(payload: str, attributes: dict[str, Any]) -> None:
"""
Send a message.
Args:
payload:
The message payload.
attributes:
Additional attributes to be sent alongside the message.
"""


# undocumented argument with the same name as a section
def should_fail(payload, Args):
"""
Send a message.
Args:
payload:
The message payload.
"""


# documented argument with the same name as a section
def should_not_fail(payload, Args):
"""
Send a message.
Args:
payload:
The message payload.
Args:
The other arguments.
"""
149 changes: 73 additions & 76 deletions crates/ruff_linter/src/docstrings/sections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::fmt::{Debug, Formatter};
use std::iter::FusedIterator;

use ruff_python_ast::docstrings::{leading_space, leading_words};
use ruff_python_semantic::Definition;
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
use strum_macros::EnumIter;

Expand Down Expand Up @@ -130,34 +131,6 @@ impl SectionKind {
Self::Yields => "Yields",
}
}

/// Returns `true` if a section can contain subsections, as in:
/// ```python
/// Yields
/// ------
/// int
/// Description of the anonymous integer return value.
/// ```
///
/// For NumPy, see: <https://numpydoc.readthedocs.io/en/latest/format.html>
///
/// For Google, see: <https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings>
pub(crate) fn has_subsections(self) -> bool {
matches!(
self,
Self::Args
| Self::Arguments
| Self::OtherArgs
| Self::OtherParameters
| Self::OtherParams
| Self::Parameters
| Self::Raises
| Self::Returns
| Self::SeeAlso
| Self::Warns
| Self::Yields
)
}
}

pub(crate) struct SectionContexts<'a> {
Expand Down Expand Up @@ -195,6 +168,7 @@ impl<'a> SectionContexts<'a> {
last.as_ref(),
previous_line.as_ref(),
lines.peek(),
docstring.definition,
) {
if let Some(mut last) = last.take() {
last.range = TextRange::new(last.start(), line.start());
Expand Down Expand Up @@ -444,6 +418,7 @@ fn suspected_as_section(line: &str, style: SectionStyle) -> Option<SectionKind>
}

/// Check if the suspected context is really a section header.
#[allow(clippy::too_many_arguments)]
fn is_docstring_section(
line: &Line,
indent_size: TextSize,
Expand All @@ -452,7 +427,9 @@ fn is_docstring_section(
previous_section: Option<&SectionContextData>,
previous_line: Option<&Line>,
next_line: Option<&Line>,
definition: &Definition<'_>,
) -> bool {
// for function definitions, track the known argument names for more accurate section detection.
// Determine whether the current line looks like a section header, e.g., "Args:".
let section_name_suffix = line[usize::from(indent_size + section_name_size)..].trim();
let this_looks_like_a_section_name =
Expand Down Expand Up @@ -509,60 +486,80 @@ fn is_docstring_section(
// ```
// However, if the header is an _exact_ match (like `Returns:`, as opposed to `returns:`), then
// continue to treat it as a section header.
if section_kind.has_subsections() {
if let Some(previous_section) = previous_section {
let verbatim = &line[TextRange::at(indent_size, section_name_size)];

// If the section is more deeply indented, assume it's a subsection, as in:
// ```python
// def func(args: tuple[int]):
// """Toggle the gizmo.
//
// Args:
// args: The arguments to the function.
// """
// ```
if previous_section.indent_size < indent_size {
if section_kind.as_str() != verbatim {
return false;
}
if let Some(previous_section) = previous_section {
let verbatim = &line[TextRange::at(indent_size, section_name_size)];

// If the section is more deeply indented, assume it's a subsection, as in:
// ```python
// def func(args: tuple[int]):
// """Toggle the gizmo.
//
// Args:
// args: The arguments to the function.
// """
// ```
// As noted above, an exact match for a section name (like the inner `Args:` below) is
// treated as a section header, unless the enclosing `Definition` is a function and contains
// a parameter with the same name, as in:
// ```python
// def func(Args: tuple[int]):
// """Toggle the gizmo.
//
// Args:
// Args: The arguments to the function.
// """
// ```
if previous_section.indent_size < indent_size {
let section_name = section_kind.as_str();
if section_name != verbatim || has_parameter(definition, section_name) {
return false;
}
}

// If the section has a preceding empty line, assume it's _not_ a subsection, as in:
// ```python
// def func(args: tuple[int]):
// """Toggle the gizmo.
//
// Args:
// args: The arguments to the function.
//
// returns:
// The return value of the function.
// """
// ```
if previous_line.is_some_and(|line| line.trim().is_empty()) {
return true;
}
// If the section has a preceding empty line, assume it's _not_ a subsection, as in:
// ```python
// def func(args: tuple[int]):
// """Toggle the gizmo.
//
// Args:
// args: The arguments to the function.
//
// returns:
// The return value of the function.
// """
// ```
if previous_line.is_some_and(|line| line.trim().is_empty()) {
return true;
}

// If the section isn't underlined, and isn't title-cased, assume it's a subsection,
// as in:
// ```python
// def func(parameters: tuple[int]):
// """Toggle the gizmo.
//
// Parameters:
// -----
// parameters:
// The arguments to the function.
// """
// ```
if !next_line_is_underline && verbatim.chars().next().is_some_and(char::is_lowercase) {
if section_kind.as_str() != verbatim {
return false;
}
// If the section isn't underlined, and isn't title-cased, assume it's a subsection,
// as in:
// ```python
// def func(parameters: tuple[int]):
// """Toggle the gizmo.
//
// Parameters:
// -----
// parameters:
// The arguments to the function.
// """
// ```
if !next_line_is_underline && verbatim.chars().next().is_some_and(char::is_lowercase) {
if section_kind.as_str() != verbatim {
return false;
}
}
}

true
}

/// Returns whether or not `definition` is a function definition and contains a parameter with the
/// same name as `section_name`.
fn has_parameter(definition: &Definition, section_name: &str) -> bool {
definition.as_function_def().is_some_and(|func| {
func.parameters
.iter()
.any(|param| param.name() == section_name)
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw
| ^ D417
173 | """Do something.
|

D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args`
|
198 | # undocumented argument with the same name as a section
199 | def should_fail(payload, Args):
| ^^^^^^^^^^^ D417
200 | """
201 | Send a message.
|
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/pydocstyle/mod.rs
snapshot_kind: text
---
D417.py:1:5: D417 Missing argument descriptions in the docstring for `f`: `y`, `z`
|
Expand Down Expand Up @@ -65,3 +64,12 @@ D417.py:155:5: D417 Missing argument description in the docstring for `select_da
156 | query: str,
157 | args: tuple,
|

D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args`
|
198 | # undocumented argument with the same name as a section
199 | def should_fail(payload, Args):
| ^^^^^^^^^^^ D417
200 | """
201 | Send a message.
|
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw
| ^ D417
173 | """Do something.
|

D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args`
|
198 | # undocumented argument with the same name as a section
199 | def should_fail(payload, Args):
| ^^^^^^^^^^^ D417
200 | """
201 | Send a message.
|
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw
| ^ D417
173 | """Do something.
|

D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args`
|
198 | # undocumented argument with the same name as a section
199 | def should_fail(payload, Args):
| ^^^^^^^^^^^ D417
200 | """
201 | Send a message.
|

0 comments on commit 7b487d8

Please sign in to comment.