Skip to content

Conversation

sam-mosleh
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

sam-mosleh commented Feb 9, 2023

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

# Not gonna work
asyncio.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
Copy link
Member

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.gather plus 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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

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

@Bibo-Joshi Bibo-Joshi requested a review from Poolitzer February 13, 2023 19:24
@harshil21 harshil21 added the ⚙️ type-hinting affected functionality: type-hinting label Feb 18, 2023
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
@sam-mosleh sam-mosleh requested review from Bibo-Joshi and removed request for Poolitzer February 19, 2023 10:29
@sam-mosleh
Copy link
Contributor Author

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

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@Bibo-Joshi Bibo-Joshi merged commit c6b6b0a into python-telegram-bot:master Feb 20, 2023
@Bibo-Joshi
Copy link
Member

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

@sam-mosleh sam-mosleh deleted the fix-create-task-type-hint branch February 21, 2023 14:09
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2023
@harshil21 harshil21 added this to the v20.2 milestone Mar 25, 2023
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔌 enhancement pr description: enhancement ⚙️ type-hinting affected functionality: type-hinting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Incorrect type hinting for Application.create_task
4 participants