achimnol / aiotools

Idiomatic asyncio utilties
https://aiotools.readthedocs.io
MIT License
153 stars 11 forks source link

Support for `msg` arg to `cancel()` added in Python 3.9 #32

Closed bdowning closed 2 years ago

bdowning commented 2 years ago

Python 3.9 added a msg arg to cancel(). If the patched parent task of a task group is cancelled with this argument, we get this error:

TypeError: _task_cancel() takes 2 positional arguments but 3 were given

This adds a test for this scenario, and then fixes it by adding an optional argument to the wrapper.

bdowning commented 2 years ago

@achimnol Pinging about this. The tests haven't been run due to needing authorization but I suspect they'll "fail" based on the head of main failing at the moment. Just bad timing on my part not getting in earlier. :)

Would there be any way to get this into a release soon? We're seeing this issue in our code that's attempting to use TaskGroups. I can probably monkey-patch around it but having a fix upstream would be great.

achimnol commented 2 years ago

@achimnol Pinging about this. The tests haven't been run due to needing authorization but I suspect they'll "fail" based on the head of main failing at the moment. Just bad timing on my part not getting in earlier. :)

Would there be any way to get this into a release soon? We're seeing this issue in our code that's attempting to use TaskGroups. I can probably monkey-patch around it but having a fix upstream would be great.

Oh, I just have been busy. Thanks for your first contribution!

achimnol commented 2 years ago

Hm... a recent update has broken the test suite. I'll inspect.

bdowning commented 2 years ago

Thanks; I'll check in later and rebase if there's a fix.

achimnol commented 2 years ago

Thanks; I'll check in later and rebase if there's a fix.

It's fixed now in the main branch. You may rebase or merge it.

bdowning commented 2 years ago

OK, rebased! Sorry for the wait.

codecov[bot] commented 2 years ago

Codecov Report

Merging #32 (c7c88ed) into main (8d8273f) will increase coverage by 0.25%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   78.48%   78.74%   +0.25%     
==========================================
  Files          11       11              
  Lines         939      941       +2     
==========================================
+ Hits          737      741       +4     
+ Misses        202      200       -2     
Impacted Files Coverage Δ
src/aiotools/taskgroup.py 91.66% <100.00%> (+1.30%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8d8273f...c7c88ed. Read the comment docs.

bdowning commented 2 years ago

Done.

It looks like every time I push it goes back to saying "workflow awaiting approval" and the CI checks don't run. Since there's also no local scriptage (that I can see anyway) to run equivalent checks to what CI will do this makes it pretty hard to know that things like the above are missing for contributors. (I'd cobbled together how to get tests to run by looking at the Github Actions files, but that's not ideal.) I think having one or the other (i.e. either a "push to run CI" model that doesn't have a several-hours cycle time or a "here's a script to run to do all the checks locally" model) would be beneficial.

achimnol commented 2 years ago

Done.

It looks like every time I push it goes back to saying "workflow awaiting approval" and the CI checks don't run. Since there's also no local scriptage (that I can see anyway) to run equivalent checks to what CI will do this makes it pretty hard to know that things like the above are missing for contributors. (I'd cobbled together how to get tests to run by looking at the Github Actions files, but that's not ideal.) I think having one or the other (i.e. either a "push to run CI" model that doesn't have a several-hours cycle time or a "here's a script to run to do all the checks locally" model) would be beneficial.

This is the GitHub's default setting to prevent potential abusing of executing arbitrary codes in the PR. If your PR is merged once, then it won't ask explicit permission from the next contributions.

bdowning commented 2 years ago

Fair enough.

Pushed a fix for the create_task use and Python 3.6. Hopefully that's the last of it.

achimnol commented 2 years ago

Thanks for the contribution!