-
-
Notifications
You must be signed in to change notification settings - Fork 800
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
Added function prototype for BaseChannelLayer #2112
Conversation
Refactored code according to linter. Should be ready now! Thanks. |
Can a maintainer please review my changes? Thank you so much! |
Hi @jophy-ye I've looked at this. I'm not immediately keen, so I'm trying to think over what I actually think here. We don't have typing but I'd lean to a Protocol, rather than null implementations if I really wanted this. (As I say, currently not sure.) |
@carltongibson Thanks. In my opinion, this is more than a null implementation: it’s extremely common to have a Redis channel layer in production and an in-memory channel for local development. I understand that channels does not accept typing at the moment, but I’d say some developers would like to (in their projects). Having a function prototype in the base class could help, but if this doesn’t fit, I’m happy to close this PR. |
@jophy-ye Yes, I'm still pondering it. Please allow me a cycle for that. |
Should we add typing? Separate discussion of course but could solve this class of problems |
@bigfootjon I'm in two minds about that. 😅 Maybe we should, but...
So still pondering that too 😅 |
On the other hand |
I’m sorry. Could anyone please share with me the difference between typing and a protocol? EDIT: IMO, complete typing is much more than protocol: there’s the use of type annotation everywhere throughout the code base. However, protocol is sort of the interface of classes, so I’d say my code constitutes a (more outward-facing) protocol effort for anybody writing their channel layer or using type annotation in their code, i.e., a middle ground between no typing and complete typing. |
@jophy-ye There is no difference... You define a protocol and then use it in your type annotations, so it's part of the typing effort. @bigfootjon Happy to dabble here if you're keen. I'd suggest adding some hints incrementally, starting with obvious and clean ones, and then seeing how that goes. |
a553056
to
35072e1
Compare
I'm fine with this as-is. @carltongibson since you might have other thoughts here I'll defer to you |
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.
OK let's have it. Thanks @jophy-ye
Added prototypes like
send()
,receive()
, etc. toBaseChannelLayer
, the base class of all channel layers. These are necessary functions for any channel layer, according to the docs.This helps intellisense when type hints are present, and assists developers writing their own channel.