Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clarify that D417 only checks docstrings with an arguments section #16494

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

MichaReiser
Copy link
Member

Summary

This came up in #16477

It's not obvious from the D417 rule's documentation that it only checks docstrings
with an arguments section. Functions without such a section aren't checked.

This PR tries to make this clearer in the documentation.

@MichaReiser MichaReiser added the documentation Improvements or additions to documentation label Mar 4, 2025
Copy link
Contributor

github-actions bot commented Mar 4, 2025

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 2 project errors)

RasaHQ/rasa (error)

Failed to clone RasaHQ/rasa: error: RPC failed; curl 92 HTTP/2 stream 5 was not closed cleanly: CANCEL (err 8)
error: 2876 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output

zulip/zulip (error)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

Failed to clone zulip/zulip: error: RPC failed; curl 92 HTTP/2 stream 5 was not closed cleanly: CANCEL (err 8)
error: 4078 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser requested a review from ntBre March 5, 2025 16:52
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

I added one comment, but thanks for doing this!

Comment on lines 1170 to 1173
/// This rule does not check functions without any arguments sections (e.g. `Args`).
///
/// This rule is enabled when using the `google` convention, and disabled when
/// using the `pep257` and `numpy` conventions.
/// Note that this rule only checks docstrings with an arguments (e.g. `Args`) section.
/// Docstrings without any arguments sections
Copy link
Contributor

Choose a reason for hiding this comment

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

This line looks like it might have gotten cut off somehow. And did you want to remove the note about the google convention? It is mentioned earlier in the docs, but this seems to add a little more clarification.

I think the change on 1170 might be enough here. (actually I slightly prefer line 1172, but either of those on line 1170 would be great)

I can't add a suggestion on deleted lines, but my suggestion is:

/// Note that this rule only checks docstrings with an arguments (e.g. `Args`) section.
///
/// This rule is enabled when using the `google` convention, and disabled when
/// using the `pep257` and `numpy` conventions.

Copy link
Member Author

Choose a reason for hiding this comment

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

And did you want to remove the note about the google convention?

Definitely not. Thanks for catching this!

@MichaReiser MichaReiser force-pushed the micha/d417-documentation branch from 7cab740 to 59dd160 Compare March 6, 2025 09:45
@MichaReiser MichaReiser enabled auto-merge (squash) March 6, 2025 09:45
@MichaReiser MichaReiser merged commit a25be46 into main Mar 6, 2025
20 checks passed
@MichaReiser MichaReiser deleted the micha/d417-documentation branch March 6, 2025 09:49
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants