Skip to content

Some publicly exposed functions/properties have no documentation #3226

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

Open
33 tasks
A5rocks opened this issue Mar 20, 2025 · 4 comments
Open
33 tasks

Some publicly exposed functions/properties have no documentation #3226

A5rocks opened this issue Mar 20, 2025 · 4 comments

Comments

@A5rocks
Copy link
Contributor

A5rocks commented Mar 20, 2025

This issue should only really be looked at after #3221 is closed (because this is mainly about what people see in an editor, rather than our documentation). The following attributes are listed in our _type_completeness.json as not having a docstring:

  • trio._unix_pipes.FdStream.send_all
  • trio._unix_pipes.FdStream.wait_send_all_might_not_block
  • trio._unix_pipes.FdStream.receive_some
  • trio._unix_pipes.FdStream.close
  • trio._unix_pipes.FdStream.aclose
  • trio._unix_pipes.FdStream.fileno
  • trio._core._io_epoll._EpollStatistics
  • trio._channel.MemoryReceiveChannel
  • trio._channel.MemoryChannelStatistics
  • trio._channel.MemorySendChannel
  • trio._core._run.Task
  • trio._socket.SocketType
  • trio._highlevel_socket.SocketStream.send_all
  • trio._highlevel_socket.SocketStream.wait_send_all_might_not_block
  • trio._highlevel_socket.SocketStream.send_eof
  • trio._highlevel_socket.SocketStream.receive_some
  • trio._highlevel_socket.SocketStream.aclose
  • trio._subprocess.HasFileno.fileno
  • trio._sync.AsyncContextManagerMixin
  • trio._sync._HasAcquireRelease.acquire
  • trio._sync._HasAcquireRelease.release
  • trio._sync._LockImpl
  • trio._core._local._NoValue
  • trio._core._local.RunVarToken
  • trio.socket.gaierror
  • trio.socket.herror
  • trio._core._mock_clock.MockClock.start_clock
  • trio._core._mock_clock.MockClock.current_time
  • trio._core._mock_clock.MockClock.deadline_to_sleep_time
  • trio.testing._raises_group._ExceptionInfo.exconly
  • trio.testing._raises_group._ExceptionInfo.errisinstance
  • trio.testing._raises_group._ExceptionInfo.getrepr
  • trio.testing._raises_group.RaisesGroup.expected_type

I expect most of these are trivial. There's a couple groups I noticed while copying them over:

  • some aren't actually publicly exposed and so we should update check_type_completeness.py to filter them out (e.g. _HasAcquireRelease)
  • some should really just inherit documentation from parents (e.g. SocketStream.wait_send_all_might_not_block). I don't know if pyright supports anything like that but it should

I think some of the logic from check_type_completeness.py is kind of strange: it shouldn't filter out docstrings that are available at runtime because... pyright still can't see those?

@jakkdl
Copy link
Member

jakkdl commented Mar 31, 2025

I think some of the logic from check_type_completeness.py is kind of strange: it shouldn't filter out docstrings that are available at runtime because... pyright still can't see those?

So what it does is:

  1. Check if pyright sees the docstring
  2. if pyright doesn't see the docstring, try to runtime import it
    a. if we fail to runtime import it.. it's usually for OS-specific reasons, so we ignore a bunch of objects here
  3. check if it has a docstring at runtime

If neither pyright nor runtime sees the docstring (and it's not Path) then it emits an error.

It might be that comments/docstrings need clarification, but I don't think the logic is wrong.

@A5rocks
Copy link
Contributor Author

A5rocks commented Mar 31, 2025

I think that's misguided. Like it makes sense but accomplishes an incorrect goal. Either the goal is:

  • everything public has a docstring when you hover over it in an editor (<- I was assuming this previously)
  • the docs are up-to-date

In the first case we shouldn't care about if the docstrings are available at runtime, I think. (I don't use VSCode so I can't say whether it gets docstrings at runtime, but I assume not.) In the second, we now have a better check so we can disable this. I suppose the current logic makes sense if we're trying to accomplish "you can call help() on all public symbols and all referenced symbols" but I think that test could probably be written down better.

@jakkdl
Copy link
Member

jakkdl commented Mar 31, 2025

Hah, I actually read a lot of docstrings in the live prompt with help() when coding (as I don't have hover-to-get-docstring configured).
Ultimately the current logic came from "oh pyright is complaining about a bunch of stuff that in fact does have docstrings, it just fails to see them for complicated reasons"

But it sounds like the test should be rewritten to not put both of those in the same bucket, since we both want to support static tools and help(). And then we have the sphinx logic for making sure docs is good.

@CoolCat467
Copy link
Member

Hah, I actually read a lot of docstrings in the live prompt with help() when coding (as I don't have hover-to-get-docstring configured).

But it sounds like the test should be rewritten to not put both of those in the same bucket, since we both want to support static tools and help().

Likewise, I find using help() invaluable, and I agree.

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