-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[channels] Make URLRouter.routes a Collection #15080
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
|
I think it makes sense to migrate from list to Collection. If you have time, could you also help check whether other places or functionalities could be updated to use Collection as well? |
`list` is invariant, which made this type inconvenient in practice. Each of the routes is either a pattern or another router.
ff63966 to
89dcb62
Compare
|
Good point - this is only iterated over, not indexed, so I only found a couple of other places in |
9a80b21 to
79675e2
Compare
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
Looks good to me now. |
|
To make it easier for the core developers to track and approve this PR, could you create an issue and link this PR to it, @cjwatson? |
|
And I think you can make the title a bit more generic since the changes are varied. |
srittau
left a comment
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.
Thanks! Notes below.
| routes: Collection[_ExtendedURLPattern | URLRouter] | ||
|
|
||
| def __init__(self, routes: list[_ExtendedURLPattern | URLRouter]) -> None: ... | ||
| def __init__(self, routes: Collection[_ExtendedURLPattern | URLRouter]) -> None: ... |
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.
The typeshed standard for for ... in loops is Iterable. In fact, Collection won't work:
from collections.abc import Container
x: Container[int] = []
for y in x: # error: "Container[int]" has no attribute "__iter__" (not iterable)
reveal_type(y) # note: Revealed type is "Any"| async def await_many_dispatch( | ||
| consumer_callables: list[Callable[[], Awaitable[ASGIReceiveCallable]]], dispatch: Callable[[dict[str, Any]], Awaitable[None]] | ||
| consumer_callables: Sequence[Callable[[], Awaitable[ASGIReceiveCallable]]], | ||
| dispatch: Callable[[dict[str, Any]], Awaitable[None]], |
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.
Since the return value of dispatch is ignored, it can be an arbitrary value and doesn't have to be None:
| dispatch: Callable[[dict[str, Any]], Awaitable[None]], | |
| dispatch: Callable[[dict[str, Any]], Awaitable[object]], |
| @@ -1,13 +1,19 @@ | |||
| from collections.abc import Collection | |||
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.
As above.
For reference, we try to cut back on the use of pseudo-protocols like |
listis invariant, which made this type inconvenient in practice. Each of the routes is either a pattern or another router.