Skip to content
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

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

jophy-ye
Copy link
Contributor

@jophy-ye jophy-ye commented Aug 10, 2024

Added prototypes like send(), receive(), etc. to BaseChannelLayer, 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.

@jophy-ye
Copy link
Contributor Author

Refactored code according to linter. Should be ready now! Thanks.

@jophy-ye
Copy link
Contributor Author

Can a maintainer please review my changes? Thank you so much!

@carltongibson
Copy link
Member

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.)

@jophy-ye
Copy link
Contributor Author

jophy-ye commented Aug 17, 2024

@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.

@carltongibson
Copy link
Member

@jophy-ye Yes, I'm still pondering it. Please allow me a cycle for that.

@bigfootjon
Copy link
Collaborator

We don't have typing but I'd lean to a Protocol

Should we add typing? Separate discussion of course but could solve this class of problems

@carltongibson
Copy link
Member

@bigfootjon I'm in two minds about that. 😅

Maybe we should, but...

  1. Django doesn't, so... (But perhaps we could show the way)
  2. Typing has come on since then — Protocols not least — but last time it was looked at, the annotations weren't pretty (in many places) so I'm reluctant to commit to a "Yes" and then find we're still in that same boat.

So still pondering that too 😅

@bigfootjon
Copy link
Collaborator

On the other hand asgiref makes heavy use of typing (including protocols) so there's precedence for django projects to use types

@jophy-ye
Copy link
Contributor Author

jophy-ye commented Aug 26, 2024

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.

@carltongibson
Copy link
Member

@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.

@jophy-ye jophy-ye force-pushed the base-channel-interface branch from a553056 to 35072e1 Compare October 11, 2024 05:26
@bigfootjon
Copy link
Collaborator

I'm fine with this as-is. @carltongibson since you might have other thoughts here I'll defer to you

Copy link
Member

@carltongibson carltongibson left a 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

@carltongibson carltongibson merged commit d384355 into django:main Oct 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants