Skip to content

Fix Application.create_task type hinting #3543

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

Merged

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