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.

Sign up for GitHub

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

[pydocstyle] Handle arguments with the same names as sections (D417) #16011

Merged
merged 11 commits into from
Feb 11, 2025
Merged

[pydocstyle] Handle arguments with the same names as sections (D417) #16011

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
90c8b64
add failing regression test
ntBre Feb 7, 2025
4a87ab1
pass tests
ntBre Feb 7, 2025
7c39487
clean up unused method
ntBre Feb 7, 2025
c9a0f86
more test cases, second failing
ntBre Feb 7, 2025
05717d1
compare section names to known arguments
ntBre Feb 7, 2025
2bcd655
update snaps
ntBre Feb 7, 2025
79cff7f
allow too_many_arguments
ntBre Feb 7, 2025
308c49b
use Docstring::definition
ntBre Feb 7, 2025
7212177
linear search instead of hashset
ntBre Feb 9, 2025
c4fe958
add example for verbatim parameter name
ntBre Feb 9, 2025
0bc81b9
move docs
ntBre Feb 10, 2025
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
18 changes: 17 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,20 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a test where one of the function arguments is named like a section header? I'm curious what happens if that argument is undocumented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what you had in mind? I added these two cases, both of which cause D417. The first one is right, Args is not documented, but the second is still a false positive that I can keep working on.

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

    Args:
        payload:
            The message payload.
    """


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

    Args:
        payload:
            The message payload.

        Args:
            The other arguments.
    """

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I had in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These cases should work now! I initially deleted the inner conditional here

ruff/crates/ruff_linter/src/docstrings/sections.rs

Lines 525 to 529 in 1f7a29d

if previous_section.indent_size < indent_size {
if section_kind.as_str() != verbatim {
return false;
}
}

which worked well for D417 but broke other rules like D214 - overindented-section. So I went for the more involved approach of passing down a Definition and extracting known parameter names from it.

One other thing I considered is just searching for a known parameter at line 514 instead of collecting a HashSet up front.

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.
"""
122 changes: 46 additions & 76 deletions crates/ruff_linter/src/docstrings/sections.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,34 +130,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 @@ -509,57 +481,55 @@ 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.
// """
// ```
ntBre marked this conversation as resolved.
Show resolved Hide resolved
if previous_section.indent_size < indent_size {
if section_kind.as_str() != verbatim {
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;
}
}
}
Expand Down
Loading