Fix Application.create_task type hinting

[sam-mosleh]

Closes#3540

---

Checklist for PRs

• Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
• Documented code changes according to the CSI standard
• Added myself alphabetically to AUTHORS.rst (optional)

[Bibo-Joshi]

Awaitable was explicitly removed from the type hints in python/typeshed#6779, so I don't really think that we should re-add it. Why Future is not listed in the annotations is not quite clear to me either. IMO we should first investigate if that's a shortcoming or concious decision in typeshed before making any moves here.

[sam-mosleh]

asyncio.create_task only accepts Coroutines, and Futures are not acceptable:

# Not gonna workasyncio.create_task(
asyncio.gather(
...
)
)

So in order to make a Task out of it, you have to await it inside a coroutine which is happening in #L994-L1031

[Bibo-Joshi]

Aha! Thanks for the finding :) Them I'm good with changing to Awaitable. I'd like to have a new unit test though that explicitly check that Future is supported by Application.create_task. Could you add that to test_application.py? Please also add to the Note in the docs of Application.create_task a bullet points that clarifies that support for Future is offered in addition to the functionality of asyncio.create_task.

Moreover, I realize that Application.create_task does in fact not support generators - in contrast to asyncio.create_task. This can be fixed by changing return await coroutine to return await asyncio.create_task(coroutine) in __create_task_callback. That way, Application.create_task would fully cover the functionality of asyncio.gatherplus also supporting Future.

Would you like to add this to your PR? this would include

• A unit test that makes sure that generators work
• Updating the type hints & docs of Application.create_task accordingly

If you'd rather like to keep your PR as-is, that's perfectly fine. It can go in a follow-up PR, then.

PS: the type completeness check can't cope with PRs from external repos yet. before merging we should check the type completeness manually

[sam-mosleh]

Is supporting an edge-case (generators) necessary for the Application.create_task because the method is already different than asyncio.create_task by supporting Futures?

[Bibo-Joshi]

I think it would be benefitial, yes. The idea behind Application.create_task was to provide a drop-in replacement for asyncio.create_task that plays will with the PTB framework. If it can do a bit more than asyncio.create_task that's okay, but it should be able to do everything that asyncio.create_task can do IMO.

[sam-mosleh]

@Bibo-Joshi Since I'm not familiar with the workflows, I'll leave the versioning and documentation of Application.create_task to you.

[sam-mosleh]

I'm not sure why, but it appears that the Codecov upload token is missing, and because of that, tests are currently failing.

[Bibo-Joshi]

Thanks for the updates! I left some small comments :)
Codecove sometime has a hiccup - rerunning the tests fixed it …

[sam-mosleh]

@Bibo-Joshi I apologize for the misclick in my request for a review.

[Poolitzer]

Changes LGTM

[Bibo-Joshi]

Thank you for your contribution @sam-mosleh ! :)