FEAT: type hint #417
FEAT: type hint #417
Conversation
|
i'm yet to take a good look at this PR |
|
@amirreza8002 best |
iwakitakuma33
force-pushed
the
feat/type_hint
branch
from
c5e95d6 to
4da1969
Compare
| from redis.asyncio.connection import ConnectionPool | ||
| from redis.asyncio.client import Redis |
There was a problem hiding this comment.
@amirreza8002
Shouldn't I also use Redis and ConnectionPool?
There was a problem hiding this comment.
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
channels_redis/core.py
Outdated
| if TYPE_CHECKING: | ||
| from redis.asyncio.connection import ConnectionPool | ||
| from redis.asyncio.client import Redis | ||
| from .core import RedisChannelLayer |
There was a problem hiding this comment.
why is this imported?
i don't see any use for it
channels_redis/core.py
Outdated
| self._lock = asyncio.Lock() | ||
| self.channel_layer = channel_layer | ||
| self._connections = {} | ||
| self._connections: "Dict[int, Redis]" = {} |
There was a problem hiding this comment.
i would just import typing.Dict and go with that instead of strings
same thing for other places where this happens
There was a problem hiding this comment.
i don't mean to do self._connections: typing.Dict[int, "Redis"]
but rather
from typing import Dict
self._connections: Dict[int, "Redis"]
channels_redis/core.py
Outdated
| @@ -143,33 +155,33 @@ def __init__( | |||
| # Number of coroutines trying to receive right now | |||
| self.receive_count = 0 | |||
| # The receive lock | |||
| self.receive_lock = None | |||
| self.receive_lock: "Optional[asyncio.Lock]" = None | |||
There was a problem hiding this comment.
typing.Optional would be a better
same thing for other places where this happens
channels_redis/core.py
Outdated
| ) | ||
| # Detached channel cleanup tasks | ||
| self.receive_cleaners = [] | ||
| self.receive_cleaners: "List[asyncio.Task]" = [] |
channels_redis/core.py
Outdated
| assert self.require_valid_group_name(group), True | ||
| assert self.require_valid_channel_name(channel), True |
There was a problem hiding this comment.
i think this name change should be in another commit
same thing for other places where this happens
| _channels_over_capacity = -1 | ||
| try: | ||
| _channels_over_capacity = float(channels_over_capacity) | ||
| except Exception: | ||
| pass | ||
| if _channels_over_capacity > 0: |
There was a problem hiding this comment.
am not sure what's happening here
but i think it should be in another commit
There was a problem hiding this comment.
connection.eval is inferred to return await[str] | str.
By converting it to float, it can be compared with 0.
channels_redis/pubsub.py
Outdated
| @@ -91,11 +95,11 @@ class RedisPubSubLoopLayer: | |||
|
|
|||
| def __init__( | |||
| self, | |||
| hosts=None, | |||
| hosts: "Union[Iterable, str, bytes, None]" = None, | |||
channels_redis/serializers.py
Outdated
| raise ValueError( | ||
| "symmetric_encryption_keys must be a list of possible keys" | ||
| ) | ||
| keys = cast("Iterable[Union[str, Buffer]]", symmetric_encryption_keys) |
| 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 |
There was a problem hiding this comment.
the two names are different than each other (one has two "__")
There was a problem hiding this comment.
I intentionally used a different name for avoiding PylancereportRedeclaration.
channels_redis/serializers.py
Outdated
| message = json.dumps(message, *args, **kwargs) | ||
| return message.encode("utf-8") | ||
|
|
||
| from_bytes = staticmethod(json.loads) | ||
|
|
||
|
|
||
| # code ready for a future in which msgpack may become an optional dependency | ||
| MsgPackSerializer: "Optional[Type[BaseMessageSerializer]]" = None |
There was a problem hiding this comment.
i don't think this is correct
also why a string?
There was a problem hiding this comment.
@amirreza8002
thanks for your review.
Sorry for one commit.
I've fixed it.
|
hi @bigfootjon |
#412