-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Django channels stubs #13939
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
Django channels stubs #13939
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
040b12e to
2edbaf3
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
Hi @sobolevn, |
This comment has been minimized.
This comment has been minimized.
sobolevn
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.
I have some concerns about adding django-channels to typeshed. I would like to discuss them in detail here:
- Since
django-stubsdoes not supportpyrightand other type checkers - what will be the result of adding these types here, wherepyrightandpytypeare also included in the CI? - What about different django versions? Does
django-channelshave any difference between them? We (django-stubs) only support the latest django version - Does
django-channelsneed any extra features? Would it benefit from a custom mypy plugin?
|
There is a very good example of usage of Channels here.
From the way I've been using Channels, it will not pull in anything that will not work properly with Pyright such as an app's models.
They only check for
I do not believe so. It is a somewhat active project so I think the typing should go in the project itself rather than here. |
sobolevn
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.
(not a full review)
I agree with @Tatsh, and I am creating an extension package for Channels. It looks like
As I mentioned in your code feedback, I think there’s no significant difference between versions because Channels only relies on some stable Django imports. I will check and aim to support at least Django 4.2 if possible. For that, I think requiring
I think not. However, one useful addition to the main @Tatsh, I used to think about adding type annotations directly to Channels instead of creating a stubs package, but there’s a comment from carltongibson mentioning that type hints have been rejected in Django. So, Channels will likely follow that approach — that’s the main reason for creating the stubs. |
This comment has been minimized.
This comment has been minimized.
24e066b to
e80bd03
Compare
This comment has been minimized.
This comment has been minimized.
e80bd03 to
0f45154
Compare
This comment has been minimized.
This comment has been minimized.
0f45154 to
4690c72
Compare
This comment has been minimized.
This comment has been minimized.
sobolevn
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.
(not a full review)
This comment has been minimized.
This comment has been minimized.
|
Hi @sobolevn, I have updated the code following your suggestions and left a small question related to the dispatch function in my above comment. I hope you can have a quick look at that to see if it's okay or not. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi @sobolevn and @carltongibson, after implementing the fixes based on your feedback, do you think the code is ready to merge now, or should it be reviewed more carefully? |
|
Hi @srittau, @brianschubert, |
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! A few more remarks below, mostly around non-documented Anys.
af2e62a to
3e85641
Compare
This comment has been minimized.
This comment has been minimized.
|
Hi @srittau, all of your feedback has been addressed. When you have time, could you please take another look? Thank you very much! |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Brian Schubert <[email protected]>
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 and sorry for the delay!
|
Hi @srittau, thank you for the approval! I just have one last question — should I update this branch by merging the main branch into it, rebasing it onto the latest main, or just leave it as is and merge it directly? |
|
Diff from mypy_primer, showing the effect of this PR on open source code: strawberry (https://github.com/strawberry-graphql/strawberry)
- strawberry/channels/handlers/base.py:14: error: Cannot find implementation or library stub for module named "channels.consumer" [import-not-found]
- strawberry/channels/handlers/base.py:15: error: Cannot find implementation or library stub for module named "channels.generic.websocket" [import-not-found]
+ ...typeshed_to_test/stdlib/builtins.pyi:137: note: "__init_subclass__" of "object" defined here
+ strawberry/channels/handlers/base.py:57: error: Incompatible types in assignment (expression has type "ChannelsLayer | None", base class "AsyncConsumer" defined the type as "BaseChannelLayer") [assignment]
+ strawberry/channels/handlers/base.py:66: error: Argument 1 of "dispatch" is incompatible with supertype "AsyncConsumer"; supertype defines the argument type as "dict[str, Any]" [override]
+ strawberry/channels/handlers/base.py:66: note: This violates the Liskov substitution principle
+ strawberry/channels/handlers/base.py:66: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ strawberry/channels/handlers/base.py:77: error: Argument 1 to "dispatch" of "AsyncConsumer" has incompatible type "ChannelsMessage"; expected "dict[str, Any]" [arg-type]
- strawberry/channels/testing.py:13: error: Cannot find implementation or library stub for module named "channels.testing.websocket" [import-not-found]
+ strawberry/channels/handlers/ws_handler.py:133: error: Argument 1 to "run" of "AsyncBaseHTTPView" has incompatible type "GraphQLWSConsumer[Context, RootValue]"; expected "GraphQLWSConsumer[None, None]" [arg-type]
- strawberry/channels/handlers/http_handler.py:22: error: Cannot find implementation or library stub for module named "channels.db" [import-not-found]
- strawberry/channels/handlers/http_handler.py:23: error: Cannot find implementation or library stub for module named "channels.generic.http" [import-not-found]
+ strawberry/channels/handlers/http_handler.py:200: error: "BaseGraphQLHTTPConsumer" has no attribute "run" [attr-defined]
+ strawberry/channels/handlers/http_handler.py:207: error: Argument "headers" to "send_headers" of "AsyncHttpConsumer" has incompatible type "dict[bytes, bytes]"; expected "Iterable[tuple[bytes, bytes]] | None" [arg-type]
+ strawberry/channels/handlers/http_handler.py:360: error: Return type "Coroutine[Any, Any, ChannelsResponse | MultipartChannelsResponse]" of "run" incompatible with return type "ChannelsResponse" in supertype "SyncBaseHTTPView" [override]
- strawberry/channels/router.py:14: error: Cannot find implementation or library stub for module named "channels.routing" [import-not-found]
|
|
We always squash merge. For future reference: Please don't force push during development, that makes reviewing harder. |
Stubs package for Django channels
Ref: