-
Notifications
You must be signed in to change notification settings - Fork 138
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
Used TypeVarTuple and ParamSpec in several places #652
Conversation
|
Sounds like we should just slap a |
closes #560 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
i compared this to my old implementation1: the main differences seem to be be that while it looks like we both viewed this as a good opportunity to add fuller type-checking to various AnyIO internals (rather than just modifying the public API), you added type-checking in some additional places that i didn't, and i added type-checking in some additional places that you didn't.
i don't believe github's interface allows me to comment suggestions on unmodified files directly on this PR, so instead here they are in the form of commits (which i have rebased atop this branch) that you can consider cherry-picking: 2f876cb...886ebdc
Footnotes
Co-authored-by: Ganden Schaffner <gschaffner@pm.me>
At least the type annotations you added to |
I've cherry-picked all the changes I thought were valid. |
i'm confused—doesn't sample code (click to expand)from typing import assert_type
import anyio
import anyio.from_thread
from anyio.abc import TaskStatus
async def foo(*, task_status: TaskStatus[int]) -> str:
task_status.started(1)
return "asdf"
with anyio.from_thread.start_blocking_portal() as portal:
fut_retval, task_status_val = portal.start_task(foo)
retval = fut_retval.result()
print(f"{retval = }")
# This is True at runtime.
print(f"{isinstance(retval, str) = }")
# But this assertion fails at type-checking time currently.
assert_type(retval, str) # error: Expression is of type "Any", not "str" [assert-type]
# However, the ``start_task`` annotation change in 05b70e2 makes it pass. the changes i made to def start_task(
self,
- func: Callable[..., Awaitable[Any]],
+ func: Callable[..., Awaitable[T_Retval]],
*args: object,
name: object = None,
- ) -> tuple[Future[Any], Any]:
+ ) -> tuple[Future[T_Retval], Any]:
"""
Start a task in the portal's task group and wait until it signals for readiness.
@@ -356,7 +357,7 @@ class BlockingPortal:
"""
- def task_done(future: Future) -> None:
+ def task_done(future: Future[T_Retval]) -> None:
if not task_status_future.done():
if future.cancelled():
task_status_future.cancel()
@@ -371,7 +372,7 @@ class BlockingPortal:
self._check_running()
task_status_future: Future = Future()
task_status = _BlockingPortalTaskStatus(task_status_future)
- f: Future = Future()
+ f: Future[T_Retval] = Future()
f.add_done_callback(task_done) (05b70e2#diff-35c3d377d2935dbe1e1e44908929406cc7647d9269d69ba04ef9d353f83b1900R337-R376) which does not declare that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple comments about the PR. I've opened them as threads so it's more organized.
Sorry for not using threads in the first place yesterday. I got excited trying to submit a review ASAP since you were waiting for me and mentioned wanting to move fast, but (in retrospect) I think I should have slowed down and organized my communication better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-submitting a couple of the other earlier suggested changes, as you may also missed them earlier.
@@ -30,6 +30,7 @@ dependencies = [ | |||
"exceptiongroup >= 1.0.2; python_version < '3.11'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in case anyone is looking at this in the future, there's some more discussion in a Gitter thread: https://matrix.to/#/!JfFIjeKHlqEVmAsxYP:gitter.im/$jPGgnv0ghzCsa33gJ2SFE1YRmFqVFoqwQIeBMMFThqw?via=gitter.im)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hooray!
note, #652 (comment) is still needed
Fixes #560.