asgiref icon indicating copy to clipboard operation
asgiref copied to clipboard

Updated timeout helper to use get_running_loop().

Open carltongibson opened this issue 2 years ago • 3 comments

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

carltongibson avatar Jul 06 '22 08:07 carltongibson

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

Kludex avatar Jul 06 '22 09:07 Kludex

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

carltongibson avatar Jul 06 '22 09:07 carltongibson

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.

carltongibson avatar Aug 08 '22 06:08 carltongibson

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.

andrewgodwin avatar Nov 29 '22 17:11 andrewgodwin

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

carltongibson avatar Nov 30 '22 12:11 carltongibson

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 avatar Dec 15 '22 09:12 carltongibson

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

carltongibson avatar Dec 15 '22 09:12 carltongibson

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 avatar Dec 15 '22 09:12 carltongibson