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

gh-102810 Add docstrings to the public facing methods of the Timeout class #102811

Merged

Conversation

JosephSBoyle
Copy link
Contributor

@JosephSBoyle JosephSBoyle commented Mar 18, 2023

Adds docstrings to the public facing methods of the Timeout class.

Adapted from the docs: https://docs.python.org/3/library/asyncio-task.html#asyncio.Timeout.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! Here are a few suggestions for improvement.

JosephSBoyle and others added 2 commits March 18, 2023 19:34
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

A few small suggestions to make the writing slightly more succinct — though some of these cause the language to diverge slightly from the docs at https://docs.python.org/3/library/asyncio-task.html#asyncio.Timeout, and we might want to keep the docs and the docstrings more-or-less in sync. (Though we generally want the docstrings to be shorter than the docs.)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Oops. Sorry, I changed my mind about some of this. :-(

I agree with Alex's suggestions.

JosephSBoyle and others added 2 commits March 19, 2023 18:16
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Cool! (@AlexWaygood, if you agree you can merge.)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Two more small things from me.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 19, 2023

I'd like to sync the docs here with some of the changes made in this PR:

.. class:: Timeout()
An :ref:`asynchronous context manager <async-context-managers>`
that limits time spent inside of it.
.. versionadded:: 3.11
.. method:: when() -> float | None
Return the current deadline, or ``None`` if the current
deadline is not set.
The deadline is a float, consistent with the time returned by
:meth:`loop.time`.
.. method:: reschedule(when: float | None)
Change the time the timeout will trigger.
If *when* is ``None``, any current deadline will be removed, and the
context manager will wait indefinitely.
If *when* is a float, it is set as the new deadline.
if *when* is in the past, the timeout will trigger on the next
iteration of the event loop.

Some of the new wordings in this PR are much nicer than what we currently have at https://docs.python.org/3/library/asyncio-task.html#asyncio.Timeout, in my opinion; it would be a shame for the docstrings to be improved but to leave the docs as they are. But perhaps this is a large enough task that it can be done in a separate PR.

Specific changes to the docs I'd like to see are:

  • The docs currently falsely imply that the constructor takes 0 arguments (it actually has a required when argument). We should probably move the discussion of the semantics of when from lines 642-648 to the main body of the docs describing the class, like we've done in this PR.
  • Lines 625-626 of the docs should be updated to be the same as the first sentence of the new class docstring
  • Line 640 of the docs can be changed to "Reschedule the timeout", same as the first (only) sentence of the new docstring for reschedule()

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@JosephSBoyle
Copy link
Contributor Author

Hiya, @AlexWaygood, w.r.t your last comment: I'm happy to do that in a separate PR or here, whichever you prefer.

IMO it might be nicer to get this PR in if you think it's ready, and then we can have a separate PR focussed on the changes to the Sphinx docs that you highlighed.

Your call:)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

IMO it might be nicer to get this PR in if you think it's ready, and then we can have a separate PR focussed on the changes to the Sphinx docs that you highlighed.

Sounds good, let's do that! Thanks for working on this :D

@AlexWaygood AlexWaygood merged commit 699cb20 into python:main Mar 19, 2023
@miss-islington
Copy link
Contributor

Thanks @JosephSBoyle for the PR, and @AlexWaygood for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-102834 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Mar 19, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 19, 2023
…cio.Timeout` (pythonGH-102811)

(cherry picked from commit 699cb20)

Co-authored-by: JosephSBoyle <48555120+JosephSBoyle@users.noreply.github.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington added a commit that referenced this pull request Mar 19, 2023
…meout` (GH-102811)

(cherry picked from commit 699cb20)

Co-authored-by: JosephSBoyle <48555120+JosephSBoyle@users.noreply.github.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
…cio.Timeout` (python#102811)

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
…cio.Timeout` (python#102811)

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants