[pydoclint] Permit yielding None in DOC402 and DOC403#13148
[pydoclint] Permit yielding None in DOC402 and DOC403#13148AlexWaygood merged 5 commits intoastral-sh:mainfrom
pydoclint] Permit yielding None in DOC402 and DOC403#13148Conversation
pydoclint] Permit yielding None in DOC402 and DOC403pydoclint] Permit yielding None in DOC402 and DOC403
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| E501 | 18 | 0 | 18 | 0 | 0 |
| NPY002 | 8 | 0 | 8 | 0 | 0 |
| DOC201 | 7 | 4 | 3 | 0 | 0 |
| DOC402 | 6 | 2 | 4 | 0 | 0 |
| C408 | 4 | 0 | 4 | 0 | 0 |
| E741 | 1 | 0 | 1 | 0 | 0 |
| PLW0127 | 1 | 0 | 1 | 0 | 0 |
| PLW0128 | 1 | 0 | 1 | 0 | 0 |
| F811 | 1 | 0 | 1 | 0 | 0 |
This comment was marked as resolved.
This comment was marked as resolved.
|
So, consider this ecosystem hit: https://github.com/apache/superset/blob/07985e2f5aa165f6868abbf88594e6d75300caae/superset/utils/core.py#L1288-L1312. The yield is "documented" there in that it's clear that the function is a context manager that only temporarily overrides a value. But there's no "Yields" section -- and I think it would make the docstring worse if they rewrote it to include a "Yields" section; but this change would mean that we'd emit DOC402 on that docstring. I think my preferred behaviour here would be:
I think this is what you suggested in #13001 (comment) (sorry that nobody responded to you there!). It's a little different from numpydoc's behaviour IIUC, but I think that's okay. |
|
(Oh and thanks for the PR! Sorry for diving straight into the review 😄) |
Would you mind clarifying this and how it interacts with the current implementation of If the recommendation is to ignore the type annotation if we determine all yields to be |
Ahh, thanks for pointing that out. Your argument makes sense; I agree that it's important for these rules to remain consistent. Your PR however creates a new inconsistency between the two rules as it currently stands, however. def foo() -> int | None:
"""Foo-y method"""
return None
def bar() -> int | None:
"""Bar-y method"""
returnWould you like to fix that as well as part of this PR, so that the rules stay consistent? |
Fixed in 79fad1c! |
CodSpeed Performance ReportMerging #13148 will not alter performanceComparing Summary
|
|
This makes sense to me, but I'll defer to @AlexWaygood to approve and merge. |
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
5993713 to
0a7a84d
Compare
fe982fe to
117fdcc
Compare
|
Thanks @tjkuson! |
Summary
Currently, docstring-extraneous-yields (DOC403) reports a diagnostic if there is a yields section in a docstring for a function that yields
Noneimplicitly; for example,This is problematic, as sometimes you want to document this yielding behaviour.
This patch permits such docstrings by now counting implicit
yield Nonestatements as yields. It then updates docstring-missing-yields (DOC402) to allow users to forego a yield section if the function is resolved to yieldNoneonly (by checking the return annotation, or by checking the yield statements if the return type is not annotated).This is similar to #13064
Closes #13001
Test Plan
cargo nextest run