Skip to content

gh-102560 Add docstrings to asyncio.TaskGroup #102565

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

Merged

Conversation

JosephSBoyle
Copy link
Contributor

@JosephSBoyle JosephSBoyle commented Mar 9, 2023

Adds docstrings to the public methods of asyncio.TaskGroup.

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.

Please fix the trailing whitespace issues that the Azure Pipelines CI check is complaining about

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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.

I desperate wish for something half the size.

@@ -135,6 +158,10 @@ async def __aexit__(self, et, exc, tb):
self._errors = None

def create_task(self, coro, *, name=None, context=None):
"""Create a new task in this group and return it.

Matches the call signature of asyncio.create_task.
Copy link
Member

Choose a reason for hiding this comment

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

Why not just

Suggested change
Matches the call signature of asyncio.create_task.
Similar to `asyncio.create_task`.

@olijeffers0n
Copy link

The part about Exceptions seems a bit long winded. Maybe just:

Any exceptions other than asyncio.CancelledError's raised within tasks will cancel all remaining tasks and exit the context. These exceptions will be combined into an ExceptionGroup and re-raised.

As a replacement?

@gvanrossum
Copy link
Member

@JosephSBoyle Can you please respond to the review comments? (PS. There's no need to merge main, even if GitHub recommends it, unless it says there's also a merge conflict.)

@JosephSBoyle
Copy link
Contributor Author

Thanks for the comments!

@AlexWaygood I believe I've fixed the whitespace issue.
@gvanrossum and @olijeffers0n I've adopted your suggestions.

Thanks for the tip on merging! 🙇‍♂️


A thought of my own, maybe we could remove the 'Similar to `asyncio.create_task' line entirely and just have:

    def create_task(self, coro, *, name=None, context=None):
        """Create a new task in this group and return it."""

I think people intuitively would expect asyncio.create_task and asyncio.TaskGroup().create_task to be similar from name alone which makes the statement redundant.

@AlexWaygood AlexWaygood dismissed their stale review March 13, 2023 22:05

Requested changes were made

@kumaraditya303
Copy link
Contributor

I defer to Alex and Guido on this.

@kumaraditya303 kumaraditya303 removed their request for review March 14, 2023 07:44
@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 14, 2023

It reads okay to me now, but I'm neither an asyncio maintainer nor an asyncio expert, so I'll defer to @gvanrossum and Kumar on whether the technical accuracy is okay and whether the style of the docstring is desirable.

Thanks for working on this!

@JosephSBoyle
Copy link
Contributor Author

Thanks for your help Alex:)
🙇‍♂️

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.

I have a slight rephrasing and some whitespace/markup nits, then LGTM.

I can commit those and land this unless you object.

@JosephSBoyle
Copy link
Contributor Author

Hi @gvanrossum, feel free to commit the changes you proposed:)

@gvanrossum gvanrossum merged commit e94edab into python:main Mar 15, 2023
@gvanrossum
Copy link
Member

Thanks! It's a real improvement to have this.

@hugovk hugovk added the needs backport to 3.11 only security fixes label Mar 15, 2023
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-102715 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 15, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 15, 2023
miss-islington added a commit that referenced this pull request Mar 15, 2023
(cherry picked from commit e94edab)

Co-authored-by: JosephSBoyle <[email protected]>
@JosephSBoyle
Copy link
Contributor Author

Happy to help!:)
🙇‍♂️

@gvanrossum
Copy link
Member

If you want another project like this, timeouts.py also needs docstrings.

@JosephSBoyle
Copy link
Contributor Author

If you want another project like this, timeouts.py also needs docstrings.

Nice, I've opened a PR to add docstrings here: #102811.

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
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.

8 participants