Conversation
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
replace str with _StrLikeT where applicable Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Sequence is a covariant unlike list, so it works with just list[tuple[str, int]]. Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
AnyStreamIdT is a TypeVar which didn't work for us Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Use Mapping instead of dict for arguments to work with covariants Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Issues were found by the tests type checking Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
244b284 to
cf6eec6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
+ Coverage 76.72% 76.79% +0.07%
==========================================
Files 129 129
Lines 34146 34269 +123
==========================================
+ Hits 26198 26318 +120
- Misses 7948 7951 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves user-facing type checking by introducing overload-based command typing (sync vs async) and expanding shared typing aliases so mypy can correctly infer return types across client variants.
Changes:
- Adds
_is_async_client: Literal[...]marker attributes to core client classes to enable overload-based sync/async typing. - Expands
valkey.typingwith additional aliases/TypedDicts used by command overloads and parsers. - Adjusts sync/async command tests to align with the stricter typing and overload expectations.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
valkey/typing.py |
Introduces sync/async client marker protocols and many new response/data type aliases and TypedDicts. |
valkey/client.py |
Adds _is_async_client marker to the synchronous Valkey client. |
valkey/cluster.py |
Adds _is_async_client marker to the synchronous ValkeyCluster client. |
valkey/asyncio/client.py |
Adds _is_async_client marker to the async Valkey client; adjusts set_response_callback typing. |
valkey/asyncio/cluster.py |
Adds _is_async_client marker to the async ValkeyCluster client. |
tests/test_commands.py |
Updates several tests to satisfy stricter typing / overload expectations. |
tests/test_asyncio/test_commands.py |
Switches to valkey.asyncio import style and adds type-narrowing asserts to satisfy mypy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| min: StringTypeT | ||
| max: StringTypeT |
There was a problem hiding this comment.
XPendingResult types min/max as StringTypeT, but XPENDING <key> <group> returns None for min/max when there are no pending entries (this is also asserted in tests). These should be typed as StringTypeT | None to match actual command behavior and avoid forcing callers/tests to fall back to Any.
| min: StringTypeT | |
| max: StringTypeT | |
| min: StringTypeT | None | |
| max: StringTypeT | None |
| class ValkeyCluster(AbstractValkey, AbstractValkeyCluster, AsyncValkeyClusterCommands): | ||
| _is_async_client: Literal[True] = True | ||
|
|
There was a problem hiding this comment.
Only ValkeyCluster was updated with _is_async_client, but this module’s pipeline() returns a ClusterPipeline instance, and ClusterPipeline currently has no _is_async_client attribute. That means it won’t satisfy AsyncClientProtocol, so the new overload-based command typing may not work on cluster pipelines. Consider adding _is_async_client: Literal[True] = True to ClusterPipeline (and any other async client-like classes here).
| return self | ||
|
|
||
| def set_response_callback(self, command: str, callback: ResponseCallbackT): | ||
| def set_response_callback(self, command: str, callback: Callable): |
There was a problem hiding this comment.
set_response_callback() now accepts callback: Callable, which is much looser than the existing ResponseCallbackT protocol (sync or async callable with (response: Any, **kwargs)). This loses type safety and is inconsistent with valkey.asyncio.cluster.ValkeyCluster.set_response_callback, which still uses ResponseCallbackT. Prefer keeping callback: ResponseCallbackT here.
| def set_response_callback(self, command: str, callback: Callable): | |
| def set_response_callback(self, command: str, callback: ResponseCallbackT): |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Pull Request check-list
Description of change
This should finally fix the annoying typing issues for the clients. With this PR, I have managed to get to zero mypy errors on
test_commands.py. This supersedes and therefore closes #265 and fixes #164.Unlike the previous approach, this time I added the type hints via overloads directly to
core.py. So there's no more multiple sources of truth. The developer is only responsible for adding at least two overloads for their function upon adding it. For the functionfoothat takes a string and returns an integer, one should write:It is inspired by a similar change in redis-py.