FEAT: type hint by iwakitakuma33 · Pull Request #417 · django/channels_redis

iwakitakuma33 · 2025-05-29T14:30:53Z

amirreza-sf80 · 2025-05-29T14:42:55Z

i'm yet to take a good look at this PR
just like to mention that redis-py has wrong type hints
so it shouldn't be used as guide to type hint this package

iwakitakuma33 · 2025-05-29T15:09:04Z

@amirreza8002
thanks for your review. I will do without redis-py!

best

iwakitakuma33 · 2025-05-29T15:22:10Z

channels_redis/core.py

+    from redis.asyncio.connection import ConnectionPool
+    from redis.asyncio.client import Redis


@amirreza8002
Shouldn't I also use Redis and ConnectionPool?

those are just classes

what i mean is, if you see something like Redis().some_method(), there is a change the type hint on some_method is wrong, and we shouldn't just put the same thing in this codebase

amirreza-sf80

this is just a quick look around, not a deep review

amirreza-sf80 · 2025-05-29T15:14:31Z

channels_redis/core.py

+if TYPE_CHECKING:
+    from redis.asyncio.connection import ConnectionPool
+    from redis.asyncio.client import Redis
+    from .core import RedisChannelLayer


why is this imported?
i don't see any use for it

amirreza-sf80 · 2025-05-29T15:16:17Z

channels_redis/core.py

        self._lock = asyncio.Lock()
        self.channel_layer = channel_layer
-        self._connections = {}
+        self._connections: "Dict[int, Redis]" = {}


i would just import typing.Dict and go with that instead of strings
same thing for other places where this happens

i don't mean to do self._connections: typing.Dict[int, "Redis"]
but rather

from typing import Dict self._connections: Dict[int, "Redis"]

amirreza-sf80 · 2025-05-29T15:17:04Z

channels_redis/core.py

        self.receive_count = 0
        # The receive lock
-        self.receive_lock = None
+        self.receive_lock: "Optional[asyncio.Lock]" = None


typing.Optional would be a better
same thing for other places where this happens

amirreza-sf80 · 2025-05-29T15:17:18Z

channels_redis/core.py

        )
        # Detached channel cleanup tasks
-        self.receive_cleaners = []
+        self.receive_cleaners: "List[asyncio.Task]" = []


typing.List

amirreza-sf80 · 2025-05-29T15:22:18Z

channels_redis/core.py

+        assert self.require_valid_group_name(group), True
+        assert self.require_valid_channel_name(channel), True


i think this name change should be in another commit
same thing for other places where this happens

amirreza-sf80 · 2025-05-29T15:23:11Z

channels_redis/core.py

+            _channels_over_capacity = -1
+            try:
+                _channels_over_capacity = float(channels_over_capacity)
+            except Exception:
+                pass
+            if _channels_over_capacity > 0:


am not sure what's happening here
but i think it should be in another commit

connection.eval is inferred to return await[str] | str.
By converting it to float, it can be compared with 0.

amirreza-sf80 · 2025-05-29T15:23:44Z

channels_redis/pubsub.py

    def __init__(
        self,
-        hosts=None,
+        hosts: "Union[Iterable, str, bytes, None]" = None,


typing.Union

amirreza-sf80 · 2025-05-29T15:25:40Z

channels_redis/serializers.py

+            raise ValueError(
+                "symmetric_encryption_keys must be a list of possible keys"
+            )
+        keys = cast("Iterable[Union[str, Buffer]]", symmetric_encryption_keys)


why a string?

amirreza-sf80 · 2025-05-29T15:28:28Z

channels_redis/serializers.py

-    class MsgPackSerializer(MissingSerializer):
+    class _MsgPackSerializer(MissingSerializer):
        exception = exc

+    MsgPackSerializer = _MsgPackSerializer
 else:

-    class MsgPackSerializer(BaseMessageSerializer):
+    class __MsgPackSerializer(BaseMessageSerializer):
        as_bytes = staticmethod(msgpack.packb)
        from_bytes = staticmethod(msgpack.unpackb)

+    MsgPackSerializer = __MsgPackSerializer


the two names are different than each other (one has two "__")

I intentionally used a different name for avoiding PylancereportRedeclaration.

amirreza-sf80 · 2025-05-29T15:28:54Z

channels_redis/serializers.py



 # code ready for a future in which msgpack may become an optional dependency
+MsgPackSerializer: "Optional[Type[BaseMessageSerializer]]" = None


i don't think this is correct
also why a string?

@amirreza8002
thanks for your review.

Sorry for one commit.
I've fixed it.

amirreza-sf80 · 2025-05-30T12:06:03Z

hi @bigfootjon
sorry for the ping
since django is now python >= 3.10, is there any plans to move this package to >=3.10 as well?
if so we could drop a lot of old syntax and take advantage of the newer typing supports

bigfootjon · 2025-06-08T21:40:45Z

Sorry for the slow reply, 2 major pieces of feedback:

We need some kind of CI on these changes. Something like how asgiref does it: https://github.com/django/asgiref/blob/main/tox.ini#L11
This PR should ONLY add type-hints, please revert all other changes like the xfail removals

iwakitakuma33 · 2025-07-04T09:19:07Z

Sorry for the slow reply, 2 major pieces of feedback:

We need some kind of CI on these changes. Something like how asgiref does it: https://github.com/django/asgiref/blob/main/tox.ini#L11

This PR should ONLY add type-hints, please revert all other changes like the xfail removals

@amirreza8002 @bigfootjon
Thanks for replying.

The xfail fix has been reverted.
The import order has been corrected.
mypy ci has been introduced.
Any has been used where necessary to reduce the number of error messages in mypy results.

iwakitakuma33 · 2025-07-15T19:55:42Z

@amirreza8002 @bigfootjon
I fixed fail of CI.

Could you approve workflow awaiting?

iwakitakuma33 · 2025-07-26T14:39:36Z

@carltongibson
thanks for approve workflow.

All tests were successful, but only the package dependency resolution error failed.
I have resolved the conflict with the main branch. Can you approve the side workflow?

amirreza-sf80 · 2025-07-27T08:46:46Z

tox.ini

why commit this?
this already exists in the code base

@amirreza8002
#417 (comment)

This is because it is necessary to check the type using ci based on the review from bigfootjon.

black flake8 isort cannot detect type hint inconsistencies.

i was misguided by how GitHub showed this
i saw a different thing
sorry about that

tip: don't merge main branches into your pr, you usually want to rebase

@amirreza8002 Thank you for your confirmation!

amirreza-sf80 · 2025-08-15T01:12:00Z

i should clarify that i am not a maintainer here, only an interested passer-by

i would note that since channels-redis is now python >= 3.9, we can get rid of typing.Dict and simply use dict (same goes forlist and the like)

iwakitakuma33 mentioned this pull request May 29, 2025

Adding type hints to channel_redis #412

Open

FEAT: type hint

4da1969

iwakitakuma33 force-pushed the feat/type_hint branch from c5e95d6 to 4da1969 Compare May 29, 2025 15:20

iwakitakuma33 commented May 29, 2025

View reviewed changes

amirreza-sf80 reviewed May 29, 2025

View reviewed changes

FIX REVIEW

21ff64d

iwakitakuma33 requested a review from amirreza-sf80 May 29, 2025 17:55

FIX: order import

10b4a42

iwakitakuma33 force-pushed the feat/type_hint branch from 28121fe to a5a1b26 Compare June 9, 2025 18:05

iwakitakuma33 force-pushed the feat/type_hint branch 2 times, most recently from 198754e to 449493c Compare July 7, 2025 17:19

TEST

1e3e55d

iwakitakuma33 force-pushed the feat/type_hint branch from 449493c to 1e3e55d Compare July 7, 2025 17:33

iwakitakuma33 and others added 2 commits July 26, 2025 23:35

Merge branch 'main' into feat/type_hint

09e71cb

CHORE: mypy deps

0052136

amirreza-sf80 reviewed Jul 27, 2025

View reviewed changes

iwakitakuma33 requested a review from amirreza-sf80 August 12, 2025 02:21

		from redis.asyncio.connection import ConnectionPool
		from redis.asyncio.client import Redis

		assert self.require_valid_group_name(group), True
		assert self.require_valid_channel_name(channel), True



		# code ready for a future in which msgpack may become an optional dependency
		MsgPackSerializer: "Optional[Type[BaseMessageSerializer]]" = None

Comments

Conversation

iwakitakuma33 commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amirreza-sf80 commented May 29, 2025

Uh oh!

iwakitakuma33 commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iwakitakuma33 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirreza-sf80 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwakitakuma33 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirreza-sf80 commented May 30, 2025

Uh oh!

bigfootjon commented Jun 8, 2025

Uh oh!

iwakitakuma33 commented Jul 4, 2025

Uh oh!

iwakitakuma33 commented Jul 15, 2025

Uh oh!

iwakitakuma33 commented Jul 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwakitakuma33 Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirreza-sf80 commented Aug 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iwakitakuma33 commented May 29, 2025 •

edited

Loading

iwakitakuma33 commented May 29, 2025 •

edited

Loading

iwakitakuma33 May 29, 2025 •

edited

Loading

iwakitakuma33 May 29, 2025 •

edited

Loading

iwakitakuma33 Jul 27, 2025 •

edited

Loading