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

Adopt Py3.8+ features: Names for asyncio tasks #3733

Closed
Tracked by #3728
Bibo-Joshi opened this issue Jun 2, 2023 · 2 comments · Fixed by #3759
Closed
Tracked by #3728

Adopt Py3.8+ features: Names for asyncio tasks #3733

Bibo-Joshi opened this issue Jun 2, 2023 · 2 comments · Fixed by #3759

Comments

@Bibo-Joshi
Copy link
Member

Python 3.8 adds a name argument to the function asyncio.create_task. This allows to give all tasks an interpretable name, which can help with debugging and also provides more transparency for users.
We should

  • revisit our code base to check all uses of asyncio.create_task and provide meaningful names
  • if possible, also extend the unit tests to assert that the name is set on the tasks. I'm not entirely sure if that's possible

A PR in this direction is very welcome. As long as #3728 is open, it should be based on the drop-py-37 branch. If you would like to send a PR, please be sure to read our contribution guide and leave a short comment so that we can assign you.

@Trifase
Copy link
Contributor

Trifase commented Jun 4, 2023

Seems to me that asyncio.create_task is called only 4 times in the whole code (3 in telegram.ext._application.py and 1 in telegram.ext._updater.py) except for tests.

I don't think that tasks in tests need to be named, to be honest...
Furthermore, it's possible to get the name of a task using get_name() and assert it, but it's really necessary?

@Bibo-Joshi
Copy link
Member Author

I don't think that tasks in tests need to be named, to be honest...

True

Furthermore, it's possible to get the name of a task using get_name() and assert it, but it's really necessary?

In general, tests should cover every behavior that we want to protect from regression and in particular this includes any documented behavior. I agree that naming of the internal tasks is not necessary to document and probably hard to test, so I'm fine with skipping that.

Another thought that I had is that we can add the name argument to our Application.create_task wrapper. It should be a keyword-only argument as in the std-lib. This must however be tested :)

@harshil21 harshil21 self-assigned this Jun 13, 2023
@harshil21 harshil21 linked a pull request Jun 17, 2023 that will close this issue
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants