-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[pydoclint
] Add docstring-missing-parameter
and docstring-extraneous-parameter
(DOC101
, DOC102
)
#13280
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #13280 will not alter performanceComparing Summary
|
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
DOC101 | 8895 | 8895 | 0 | 0 | 0 |
DOC102 | 50 | 50 | 0 | 0 | 0 |
D205 | 2 | 1 | 1 | 0 | 0 |
D212 | 2 | 1 | 1 | 0 | 0 |
D400 | 2 | 1 | 1 | 0 | 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I haven't gone through the ecossytem results but I think there are a few cases where we don't parse the parameter names correctly.
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
20 | | ||
21 | / Args: | ||
22 | | lst (list of int): A list of integers. | ||
23 | | multiplier (int): The multiplier for each element in the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highlighting a multiline-range always leads to somewhat awkward code frame rendering. I think it would be better to change the diagnostic range to the multiplier
code frame rather than the entire args section. Or is there precedence in other pydocstyle rules to highlight the entire section?
If this range is due to creating a single diagnostic for all the function's extraneous parameters than I think I prefer having multiple diagnostics instead (also makes it easier to suppress an error in case that's needed without risking silencing new extraneous parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is how I have done it previously for example DOC502. The main decision factor is that it's a bit awkward to get the range of the single entry.
...s/ruff_linter__rules__pydoclint__tests__docstring-extraneous-parameter_DOC102_google.py.snap
Outdated
Show resolved
Hide resolved
...s/ruff_linter__rules__pydoclint__tests__docstring-extraneous-parameter_DOC102_google.py.snap
Show resolved
Hide resolved
7b96439
to
4d47b2d
Compare
Hmm, there's an overlap with https://docs.astral.sh/ruff/rules/undocumented-param/. Not quiet sure how to resolve the overlap. |
|
That's fair. But the google style one is less strict than the new one. It doesn't require that you document the parameters of all functions. It only requires that the parameters match the function's parameters if there's an Arguments section. |
We could add an option to only highlight violations when a section is present. I tried something similar #13302. |
We plan to look into the conflict but it may take some time. What we could do to move ruff/crates/ruff_linter/src/rules/pydocstyle/rules/sections.rs Lines 1784 to 1846 in 281e6d9
|
I agree that the rule makes more sense here semantically (since it's about content rather than style)... though the Anyway... I think I'm comfortable adding this to |
@charliermarsh that sounds okay to me |
Thanks guys. Is this ready for merge, or are there more changes requested? |
Summary
Add
docstring-missing-parameter
anddocstring-extraneous-parameter
(DOC101
,DOC102
). These rules check that the parameters defined in a functions signature match thos defined in the docstring.Part of #12434.
Test Plan
Test cases added.