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

Some functions and properties in the documentation have no docstring #3221

Open
11 tasks
basak opened this issue Mar 10, 2025 · 7 comments
Open
11 tasks

Some functions and properties in the documentation have no docstring #3221

basak opened this issue Mar 10, 2025 · 7 comments

Comments

@basak
Copy link
Member

basak commented Mar 10, 2025

The following things lack documentation in Trio's documentation:

  • 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

These should either have documentation or not show up (e.g. the SocketStream attributes seem unnecessary, they're only in the docs because of a :undoc-members:).


At https://trio.readthedocs.io/en/latest/reference-core.html#trio.CancelScope.relative_deadline, there is no definition of the semantics of relative_deadline. Looks like it was added in #3010.

@basak
Copy link
Member Author

basak commented Mar 10, 2025

IIRC, there are some non-trivial details, such as that it's relative to the time that the cancel scope is first entered, but if changed later, it's relative to the time that the change occurred. This needs checking though - I inferred it from the code when I needed to understand it, concluded that I didn't need it, and that was a while ago. I just noticed now that the docs are still missing details so thought it might be helpful to note this here.

@A5rocks
Copy link
Contributor

A5rocks commented Mar 10, 2025

IIRC we described the behavior in relevant PR comments but I'm not completely sure if it's a complete description. Maybe tests works as a better description...

@jakkdl
Copy link
Member

jakkdl commented Mar 11, 2025

that is a pretty ugly hole in the docs indeed. The @relative_deadline property needs a docstring - the newsfragment describe the behavior before entering: "This allows initializing scopes ahead of time, but where the specified relative deadline doesn't count down until the scope is entered." but yeah need to go through tests and/or comments in #3010 to note the behavior after entering.

Would be nice if this got caught by a linter/test somewhere, but am not sure about any decent way of approaching that. The best would probably be a way of seeing the RTD HTML diff in an easy way

@jakkdl
Copy link
Member

jakkdl commented Mar 11, 2025

Hm, I was not aware of the diff view in RTD: https://trio--3197.org.readthedocs.build/en/3197/reference-core.html?readthedocs-diff=true&readthedocs-diff-chunk=1
it has some false alarms, but will try to remember it in the future. You enable it by interacting with the menu in the top right on PR doc builds

@A5rocks
Copy link
Contributor

A5rocks commented Mar 11, 2025

Would be nice if this got caught by a linter/test somewhere, but am not sure about any decent way of approaching that. The best would probably be a way of seeing the RTD HTML diff in an easy way

I don't know if there's any proxy for "this needs documentation and doesn't have it". We could require docstrings for properties (under the idea that "if it's computed it should be documented")... but I don't know, that seems a bit onerous. Maybe worth trying and seeing what fails the test?

Maybe the test should only fail if there's a getter and a setter and the class is exposed and neither getter nor setter have docstrings?

@jakkdl
Copy link
Member

jakkdl commented Mar 11, 2025

Would be nice if this got caught by a linter/test somewhere, but am not sure about any decent way of approaching that. The best would probably be a way of seeing the RTD HTML diff in an easy way

I don't know if there's any proxy for "this needs documentation and doesn't have it". We could require docstrings for properties (under the idea that "if it's computed it should be documented")... but I don't know, that seems a bit onerous. Maybe worth trying and seeing what fails the test?

Maybe the test should only fail if there's a getter and a setter and the class is exposed and neither getter nor setter have docstrings?

Haha. I don't think it needs a setter, but I think it's only a "real" problem if they're in the documentation as .. autoattribute:: relative_deadline, which means that the test should read the rst files. I suspect we have plenty properties that are only documented in the class docstring and nobody cares in the slightest about it.

(but to clarify: I don't really think this is worth the effort)

@A5rocks
Copy link
Contributor

A5rocks commented Mar 12, 2025

Idea: we could hook the autodoc_process_signature event and check everything has a docstring, warning otherwise.

We already hook the event for other reasons so it wouldn't even be that big of a code change...


Adding a small thing to autodoc_process_docstring, I find the following have no docstring:

!!!!!! 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

... maybe this is a good idea.

@A5rocks A5rocks changed the title CancelScope.relative_deadline is not documented Some functions and properties in the documentation have no docstring Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants