-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pydoclint] Implement docstring-extraneous-parameter (DOC102)
#20376
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
Conversation
2a4a7d2 to
e62a658
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| DOC102 | 66 | 66 | 0 | 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.
Thanks for your work here and for splitting off the PR! This looks great to me overall, just a few minor ideas/suggestions. Did you look into reusing any code from D417? That seemed like the only comment from the other PR that wasn't fully resolved.
I'd also like @MichaReiser to have a chance to look since he reviewed #13280 originally.
...s/ruff_linter__rules__pydoclint__tests__docstring-extraneous-parameter_DOC102_google.py.snap
Show resolved
Hide resolved
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.
Thank you! You went above and beyond my diagnostic suggestion. I think we could use that for the main diagnostic now. The code looks good to me otherwise.
However, I took a quick peek at the ecosystem results, and this one in particular looks pretty suspicious:
disnake/client.py:805:9: DOC102 These documented parameters are not in the function's signature: Example, --------, .. code-block
I know we have some open issues around sphinx docstrings, but ideally we'd eliminate these false positives at least.
This one also looks interesting, I wonder if we should consider skipping *args as well as **kwargs:
disnake/ext/commands/context.py:310:9: DOC102 Documented parameter entity is not in the function's signature
This one looks like it should have been straightforward, but we have a false positive on cls:
disnake/ext/commands/flag_converter.py:556:9: DOC102 Documented parameter cls is not in the function's signature
We'll need to go through all of the ecosystem results, but those are a few I pulled out from the disnake run.
...s/ruff_linter__rules__pydoclint__tests__docstring-extraneous-parameter_DOC102_google.py.snap
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
|
I fixed: disnake/ext/commands/context.py:310:9: DOC102 Documented parameter entity is not in the function's signature and disnake/ext/commands/flag_converter.py:556:9: DOC102 Documented parameter cls is not in the function's signature however the problem with disnake/client.py:805:9: DOC102 These documented parameters are not in the function's signature: Example, --------, .. code-block is that |
b520f96 to
16d6394
Compare
|
Thanks! Have you looked through the other ecosystem results? It looks like airflow has some YAML blocks in docstrings that we're trying to parse as parameter names (lots of
I wonder if we should be a little more permissive with the numpy example(s) section. I don't really like the look of emitting diagnostics like this:
It may help in several of these cases to filter out candidates that aren't valid Python identifiers, if we can't avoid checking them in the first place. |
...ts/ruff_linter__rules__pydoclint__tests__docstring-extraneous-parameter_DOC102_numpy.py.snap
Outdated
Show resolved
Hide resolved
I have limited the detection of parameters to those beginning with valid characters.
We can, and it is easy to change, but it seems out of scope for this PR
As far as I'm concerned this one is a problem with their formatting, they should have a space between the parameter and the type.
This is also a problem with their docstrings, they aren't using google or numpy style, so we can't parse them properly. |
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.
Thanks! I've been thinking about how to handle the false positives. I think I agree with you that these are technically issues in the ecosystem docs, but they'll still look like false positives in Ruff, so I'd like to err on the side of filtering them out.
I had a couple of ideas about identifier handling in an inline comment that I think could help with some of them.
I'm somewhat less concerned about the Example case and the inline YAML docstrings, but one idea that came to mind is if we can bail out if we're not in a known Args or Parameters section? That seems like it would filter out at least the YAML cases I looked at, which don't fall under any known section heading as far as I can tell.
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
If by YAML docstring you are referring to something like this https://github.com/apache/superset/blob/4b71adaa9c89925676e7fed6af0c56c5b0ece863/superset/dashboards/api.py#L1711, the issue is that one of the keys is |
|
@ntBre Any further comments? |
|
No, I think you've convinced me. I might do one more pass over the code, but I think we should land this. There are still a few false positives for unusual docstring formats, but the ecosystem check is also showing quite a few true positives. I think it's a helpful rule. |
|
@ntBre Sorry to be pushy, but it's been a year since I made the initial pull request, it's been reviewed 3 or 4 times, and every time the conversation just goes silent with no more comments and no merge. I'm fine with making more changes, and fine with it not being merged, but I would really appreciate some transparency about what is holding things up. Best, Auguste |
|
Sorry, just a bit behind on my notifications. This is still in my inbox to do another once-over on the code, but I didn't quite get to it today. I'll make this my first review tomorrow. |
|
@ntBre Thanks for the quick reply, and appreciate everything you guys do |
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.
Awesome, thanks for all of your work on this and for your patience with all the reviews!
I pushed one commit using TextSize a bit more heavily in the parameter parsing functions. The +1 to the length wasn't quite right for files with \r\n line endings, but we can use the full_line_end method from the LineRanges trait to help with that. Then it became a bit easier to use TextSize elsewhere and avoid some unwraps to boot.
I also wrote up a follow-up issue for the last two types of false positives in the ecosystem check: #20923. I think we should go ahead and land this, but I wanted to track those somewhere.
I'll make sure my change doesn't regress the ecosystem check, and then I'll hit merge!
…rable * origin/main: [ty] Support dataclass-transform `field_specifiers` (#20888) Bump 0.14.1 (#20925) Standardize syntax error construction (#20903) [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376) [ty] Fix panic 'missing root' when handling completion request (#20917) [ty] Run file watching tests serial when using nextest (#20918) [ty] Add version hint for failed stdlib attribute accesses (#20909) More CI improvements (#20920) [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
* main: [ty] Prefer declared type for invariant collection literals (#20927) [ty] Don't track inferability via different `Type` variants (#20677) [ty] Use declared variable types as bidirectional type context (#20796) [ty] Avoid unnecessarily widening generic specializations (#20875) [ty] Support dataclass-transform `field_specifiers` (#20888) Bump 0.14.1 (#20925) Standardize syntax error construction (#20903) [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376) [ty] Fix panic 'missing root' when handling completion request (#20917) [ty] Run file watching tests serial when using nextest (#20918) [ty] Add version hint for failed stdlib attribute accesses (#20909) More CI improvements (#20920) [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
Summary
Implement
docstring-extraneous-parameter(DOC102). This rule checks that all parameters present in a functions docstring are also present in its signature.Split from #13280, per this comment.
Part of #12434.
Test Plan
Test cases added.