Skip to content
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

Warn if there's no docstring for documented things #3222

Merged
merged 3 commits into from
Mar 24, 2025

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Mar 12, 2025

References #3221

An earlier version found these:

!!!!!! trio.CancelScope.relative_deadline
!!!!!! trio.MemorySendChannel
!!!!!! trio.MemoryReceiveChannel
!!!!!! trio.MemoryChannelStatistics
!!!!!! trio.SocketStream.aclose
!!!!!! trio.SocketStream.receive_some
!!!!!! trio.SocketStream.send_all
!!!!!! trio.SocketStream.send_eof
!!!!!! trio.SocketStream.wait_send_all_might_not_block
!!!!!! trio._subprocess.HasFileno.fileno
!!!!!! trio.lowlevel.ParkingLot.broken_by

Which at least seems right:

  • relative_deadline is from the above issue
  • SocketStream has :undoc-members: ... for no good reason??
  • didn't check the others

Anyways we could probably edit the issue above for a checklist of every one of those so we have easy issues for anyone who wants to contribute!

@A5rocks A5rocks force-pushed the experiments/warn-no-docstring branch from 408c5a3 to 8f8641c Compare March 12, 2025 23:05
@A5rocks A5rocks changed the title Warn if there's no docstring for functions Warn if there's no docstring for documented things Mar 12, 2025
@A5rocks
Copy link
Contributor Author

A5rocks commented Mar 12, 2025

I was wondering whether the pyright check for docstrings is still useful and I guess it is? If we decide it's still useful we should probably make the check type-completeness script not remove any warnings about docstrings, because AFAICT that's what it currently does?

IMO "does documentation show up on hover" is less important than "does documentation exist on RTD"

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

oh that is a much cleaner implementation than I expected.

Have you checked that outputting on logger.warning will fail CI? If so, then this looks great EDIT: yeah can be seen in https://readthedocs.org/projects/trio/builds/27488118/

@jakkdl
Copy link
Member

jakkdl commented Mar 13, 2025

I was wondering whether the pyright check for docstrings is still useful and I guess it is?

If I recall correctly it caught a missing docstring in the not too distant past, but you're right it's less important if it's not a docstring that would show up in the docs

If we decide it's still useful we should probably make the check type-completeness script not remove any warnings about docstrings, because AFAICT that's what it currently does?

it only removes known warnings in the same manner as your "UNDOCUMENTED", and looking closer at the list I'm seeing substantial overlap between them. The primary differences being check_type_completeness also warning for classes, as well as docstrings that wouldn't show up in the docs, but doesn't notice properties.

An issue tracking missing docstrings sounds like a good idea, I don't think anybody is especially aware of the stuff listed in src/trio/_tests/_check_type_completeness.json.
There's also the AsyncIOWrapper doing weird stuff in src/trio/_tests/check_type_completeness.py that's littered with # TODO.

(my personal favorite is # Manually confirmed to have docstrings but pyright doesn't see them due to export shenanigans. TODO: actually manually confirm that.) 😅

@A5rocks
Copy link
Contributor Author

A5rocks commented Mar 14, 2025

An issue tracking missing docstrings sounds like a good idea, I don't think anybody is especially aware of the stuff listed in src/trio/_tests/_check_type_completeness.json.

Yeah. TBH I think pyright stuff is lower priority than missing in docs; so maybe edit the referenced issue to be about all docs stuff, and a seperate issue about the pyright docstrings (with like, some note that "this is less important than [other issue]")

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (b6813ed) to head (2ccff05).
Report is 6 commits behind head on main.

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3222   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             124          124           
  Lines           18779        18779           
  Branches         1269         1269           
===============================================
  Hits            18779        18779           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@A5rocks A5rocks added this pull request to the merge queue Mar 24, 2025
Merged via the queue into python-trio:main with commit a19343d Mar 24, 2025
43 checks passed
@A5rocks A5rocks deleted the experiments/warn-no-docstring branch March 24, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants