Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
[3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks#133948
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[3.13] gh-133745: Fix asyncio task factory name/context kwarg breaks#133948
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f298146
to5c3c953
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is the right thing, just one refactoring suggestion.
We should ask Alice to try this (maybe after it's in the nightly builds).
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be clear, this is supposed to happeninstead of the rollback, right?
that's my understanding, yet |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Despite what the comment says, you're just removing the try/except here. Was that intentional?
Ah it looks like that in the diff, but I also added an else and removed the early return. Execution follows after the if/else and returns the task deleting it 'after' the return |
Got it. What's difficult or controversial about the news fragment? Just that we're waffling about the feature? I think it's reasonable to say something like this:
WDYT? If you agree, let's merge this. |
Misc/NEWS.d/next/Library/2025-05-14-08-13-08.gh-issue-133745.rjgJkH.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am happy with the code and the news blurb.
I'd like@kumaraditya303 to review this too, to be extra sure that we don't have to backtrack more in 3.13.5.
And don't we need something in the docs about this change? I just noticed that it looks like the previous change didn't even have a versionchanged (3.13.3) directive. We should add that retroactively, and another versionchanged (3.13.4) mentioning what we changed there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM,@graingert do you want to add the version changed entry?
Doc/library/asyncio-task.rst Outdated
@@ -238,18 +238,31 @@ Creating Tasks | |||
----------------------------------------------- | |||
.. function:: create_task(coro, *, name=None, context=None) | |||
.. function:: create_task(coro, *, name=None, context=None, eager_start=None, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh hang on these didn't change until 3.14
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Doc/library/asyncio-eventloop.rst Outdated
An optional keyword-only *eager_start* argument allows eagerly starting | ||
the execution of the :class:`asyncio.Task` at task creation time. | ||
If set to ``True`` and the event loop is running, the task will start | ||
executing the coroutine immediately, until the first time the coroutine | ||
blocks. If the coroutine returns or raises without blocking, the task | ||
will be finished eagerly and will skip scheduling to the event loop. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't believe we should documenteager_start
here at all. I doesn't require special treatment, it can just be put in**kwargs
. It looks like we added this in 3.13.4, and such feature changes in a bugfix release are frowned upon. We should only advertise**kwargs
as an unfortunate added in3.13.3 and fixed in 3.13.4.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Everything looks great! I will merge this now.
fd6a602
intopython:3.13Uh oh!
There was an error while loading.Please reload this page.
Thanks Thomas for moving this forward! Hopefully this is the end of the story for 3.13. For 3.14 I think we ought to add a bit to whatsnew/3.14.rst. |
Uh oh!
There was an error while loading.Please reload this page.