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

Updated timeout helper to use get_running_loop(). #337

Merged
merged 2 commits into from
Dec 15, 2022
Merged

Conversation

carltongibson
Copy link
Member

The get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks.

@carltongibson carltongibson marked this pull request as ready for review July 6, 2022 08:50
@Kludex
Copy link
Contributor

Kludex commented Jul 6, 2022

Should the loop parameter on timeout.__init__ be deprecated? 🤔

@carltongibson
Copy link
Member Author

@Kludex I suspect it should (but would address that separately, as it's a bigger change).

@carltongibson
Copy link
Member Author

I added a second commit deprecating the timeout() argument.

@andrewgodwin can I ask for initial guidance on the message, tests and docs you want for this? I'm not sure how you manage deprecations in asgiref, and over what timescale. Thanks.

@andrewgodwin
Copy link
Member

Wow, I totally missed that last mention, huh?

I don't think we ever had a deprecation plan, but I think we can follow the Django standard of "deprecation warnings for a while, then remove it", so in that sense this looks good.

@carltongibson
Copy link
Member Author

Thanks @andrewgodwin, no stress 🙂 Let me have a look over it, and I'll rebase etc.

@carltongibson
Copy link
Member Author

OK, so — I don't think this is worth a breaking change over.

I'll assume the next version will be 3.6.

I think the least effort approach here is to add the deprecation but not with any eye to actually removing the loop any time soon. Uncontroversially it could be removed in a v4. The loop param here is Meh (technical term) so dropping it in a 3.7 or 3.8 would be fine by me, but folks get very particular about SemVer. 🤷

asyncio has its own versions from 3.11: https://docs.python.org/3.11/library/asyncio-task.html#timeouts

... so eventually this utility just gets dropped.

@carltongibson
Copy link
Member Author

I see/recall 6a0cae0 is already merged, so small breaking changes without a major version bump are likely OK

@carltongibson
Copy link
Member Author

OK, I rebased, adding a change note. @andrewgodwin please let me know if you'd like any changes. (e.g. I could add an assert on the version number in the deprecation, so that we remember to remove it later... — I'm not this is pressing.) Thanks.

@carltongibson carltongibson mentioned this pull request Dec 15, 2022
3 tasks
Copy link
Member

@andrewgodwin andrewgodwin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@carltongibson carltongibson merged commit ea79016 into main Dec 15, 2022
@carltongibson carltongibson deleted the update-timeout branch December 15, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants