trio icon indicating copy to clipboard operation
trio copied to clipboard

Some functions and properties in the documentation have no docstring

Open basak opened this issue 10 months ago • 7 comments

The following things lack documentation in Trio's documentation:

  • [x] 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 avatar Mar 10 '25 23:03 basak

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.

basak avatar Mar 10 '25 23:03 basak

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

A5rocks avatar Mar 10 '25 23:03 A5rocks

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 avatar Mar 11 '25 09:03 jakkdl

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

jakkdl avatar Mar 11 '25 09:03 jakkdl

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?

A5rocks avatar Mar 11 '25 16:03 A5rocks

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)

jakkdl avatar Mar 11 '25 16:03 jakkdl

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 avatar Mar 12 '25 22:03 A5rocks