Variable named "args" leads to ambiguous docstring parsing #9426
Comments
|
I need to look into it, but Ruff handles it correctly when formatted as: """Test module for Ruff."""
from typing import Any
def select_data(
query: str,
args: tuple[Any],
database: str,
auto_save: bool,
) -> None:
"""Take a list of arguments to query the database and return it.
Args:
query: Query template.
args: A list of arguments.
database: Which database to connect to ("origin" or "destination").
auto_save: True for saving the query response in a CSV file.
Returns:
Returns the database data obtained from the query.
Raises:
ValueError:
When the database value does not match a supported option.
"""
print(query, args, database, auto_save) |
|
I think that pydocstyle may be silently failing for you in this case due to its lack of an explicit convention setting. If you run pydocstyle over this file: """Test module for Ruff."""
from typing import Any
def select_data(
query: str,
args: tuple[Any],
database: str,
auto_save: bool,
) -> None:
"""Take a list of arguments to query the database and return it.
Args:
query:
Query template.
args:
A list of arguments.
database:
Which database to connect to ("origin" or "destination").
auto_save:
True for saving the query response in a CSV file.
"""
print(query, args, database, auto_save)You get: |
|
And, similarly, if you remove any of the arguments from """Test module for Ruff."""
from typing import Any
def select_data(
query: str,
args: tuple[Any],
database: str,
auto_save: bool,
) -> None:
"""Take a list of arguments to query the database and return it.
Args:
args:
A list of arguments.
database:
Which database to connect to ("origin" or "destination").
auto_save:
True for saving the query response in a CSV file.
Returns:
Returns the database data obtained from the query.
Raises:
ValueError:
When the database value does not match a supported option.
"""
print(query, args, database, auto_save) |
|
The issue (IIRC) is that pydocstyle uses NumPy-style convention if it finds any NumPy-compatible section names. It's kind of confusing behavior, but e.g., if you have |
|
I think our behavior is "more correct" than pydocstyle, but we should probably also respect using a newline here since it seems consistent with the Google style guide. I'm surprised that we don't already. |
|
(Repurposed the issue to look into that newline behavior, though there may be a reason it's unsupported -- need to look into it.) |
|
Oh wow, I think we actually do support newlines there. The issue is that using an argument named Specifically: when you have I don't think we can properly support that, since it is legitimately ambiguous... I'd recommend using a less ambiguous variable name, or omitting the newline in that case, or putting |
|
(I'm gonna close since this is unfortunately as intended given the name collision, but I'm happy to explain in more detail or help with debugging in any other way.) |
|
Hello @charliermarsh, you wrote faster than I could reply. As you already confirmed, the newline after the argument name is also part of the "goggle" convention that we chose to follow, and we use it for more clarity's sake,
In my case, the argument name is args, but is not the same as *args, so I cannot use that. I'm sorry, but I don't think is ambiguous at all, in my example, the section Args is clearly there, and all the arguments are correctly indented inside the section, also the capitalization of the section name and arguments is different. And since I'm following the standard correctly, I should not be forced to rename the argument. I really think that argument naming should not be restricted, since that is not part of the docstring guideline, it's an issue of the linter confusing the names. Another thing that comes to my mind is that in the docstring there can only be 1 Args section (as far as I know), so it makes sense to think that if there is already an "Args:" found, other "args:" cannot be a section too. I just saw another issue similar to this one but using "raises" instead of "args", so other people are being affected too: I do think you can support it by adding some extra logic in Ruff. It seems that this I caused because Ruff wants to warn about a potential error in the capitalization of the Args section (the D405), so it wants to treat any "section_keyword:"-like line as a Section, but it's too indiscriminate. In my example, the Args section is established with the correct capitalization, so I think that Ruff could flag that line as the Section, remember its indentation (number of whitespaces), and treat all the following indented lines as arguments without testing the section keywords on them again until finding either a blank line, a line with same or less indentation that the previous Section header, or finishing the docstring, and that would solve the problem. Now if the section header doesn't have the correct capitalization, then Ruff can go bananas on warnings of course, Using that structured logic on the parsing, it would be more robust and even this extreme example would work, no naming limitation: Please consider it and give it a thought. I would offer to do it but I don't know how to program in Rust. =/ |
|
Addendum.
I found this weird. I tried again with pydocstyle using your same command line with the explicit convention, but I did not get any warning myself, maybe you have a different version?? I have the last one released. |
|
I'm using the same version: To clarify, you see these errors when you remove """Test module for Ruff."""
from typing import Any
def select_data(
query: str,
args: tuple[Any],
database: str,
auto_save: bool,
) -> None:
"""Take a list of arguments to query the database and return it.
Args:
query:
Query template.
args:
A list of arguments.
database:
Which database to connect to ("origin" or "destination").
auto_save:
True for saving the query response in a CSV file.
"""
print(query, args, database, auto_save)If you leave |
|
I hear you. I'm happy to re-open and look into changing the behavior here, though I'll caveat that the parsing logic is somewhat precarious -- it's derived from the same logic in pydocstyle, and is based on string parsing rather than a formal grammar. Depending on how invasive it is to fix, I may close again it's more complex than is worthwhile for this edge case. |
|
I agree that this should be considered a bug. |
|
I think I was able to fix this in #9427. |
## Summary
Given a docstring like:
```python
def func(x: int, args: tuple[int]):
"""Toggle the gizmo.
Args:
x: Some argument.
args: Some other arguments.
"""
```
We were considering the `args:` descriptor to be an indented docstring
section header (since `Args:`) is a valid header name. This led to very
confusing diagnostics.
This PR makes the parsing a bit more lax in this case, such that if we
see a nested header that's more deeply indented than the preceding
header, and the preceding section allows sub-items (like `Args:`), we
avoid treating the nested item as a section header.
Closes #9426.
|
Fixed, will go out in the next release! |
I see, thanks for explaining, I didn't fully understand before.
That's amazing, thanks a lot for the quick fix!!! If it also works with the other issue #8457, maybe you can close that one too. =) I will be eagerly waiting for your next release to update it. |
|
Oh, great call! I can close the other issue too :) |
Hello!
I usually use PyDocStyle to check my docstrings and recently found that that project was deprecated and archived, so they recommended using Ruff instead, so I installed it to test it.
Ruff version is ruff 0.1.11
This is my configuration file:
But I quickly found that Ruff was giving me warnings for docstrings that didn't had any problems, because it was confusing the "args" argument name with the "Args" section of the docstring, which generated up to 5 different warnings.
The following is a mock method that shows the problem:
For this file, Ruff gives this output:
It does not recognize the descriptions of any argument after "args" and applies the Section rules to the argument, causing all those warnings. The "args" argument is fairly common in my work, so this means that I cannot replace pydocstyle with ruff yet, which is a pity since ruff looks pretty powerful.
If I change the "args" argument name to something else or if I just change
args:toargs :, the messages go away, so it's obvious that Ruff is looking for a case insensitive "whitespace-args:"... Unfortunately, I don't know to code in Rust to try to find a fix, but a suggestion is that maybe you can put an internal flag on which once the "Args:" section is correctly found, it considers any other "args" as argument (especially those in lowercase) in the docstring.Thanks in advance!
The text was updated successfully, but these errors were encountered: