[redis] fix pipeline return types by chdsbd · Pull Request #4989 · python/typeshed

chdsbd · 2021-01-30T03:28:57Z

Pipeline is a subclass of Redis, but instead of the commands returning results, they return Pipeline.

Previously the types said Redis().pipeline().rpush("mylist") returned int. The updated types now type the response as Pipeline.

With this change the following code now type checks.

from redis import Redis
r = Redis()
list_length, _ = r.pipeline().rpush("mylist").ltrim("mylist", 0, 100).execute()

Pipeline is a subclass of Redis, but instead of the commands returning results, they return Pipeline. Previously the types incorrectly said `Redis().pipeline().rpush("mylist")` returned `int`. The updated types now type the response as `Pipeline`. With this change the following code now type checks. ``` from redis import Redis r = Redis() list_length, _ = r.pipeline().rpush("mylist").ltrim("mylist", 0, 100).execute() ```

chdsbd · 2021-01-30T03:31:06Z

-    def reset(self): ...
+    def reset(self) -> None: ...
    def multi(self) -> None: ...
-    def execute_command(self, *args, **kwargs): ...


execute_command, along with parse_response, watch, and unwatch were duplicated in the methods I copied from Redis, so I deleted these and updated the copied ones below.

chdsbd · 2021-01-30T03:32:01Z

-    def unwatch(self): ...
    def script_load_for_pipeline(self, script): ...
-
-class StrictPipeline(BasePipeline, StrictRedis): ...


StrictPipeline and BasePipeline don't exist in the library, so I think it's safe to use use the name Pipeline: https://github.com/andymccurdy/redis-py/blob/1a41cfd95a53a95b078084d8627be6b6fba3bb71/redis/client.py

This probably was leftover from an old redis-py version.

chdsbd · 2021-01-30T03:32:52Z

+    # methods from `Redis` client.
+    # arguments match `Redis`, but with some exceptions return types are
+    # `Pipeline` or `None`


Basically all the commands have the same arguments as Redis, but the return types are different.

chdsbd · 2021-01-30T03:34:32Z

+    def sort(
+        self,
+        name: _Key,
+        start: Optional[int] = ...,
+        num: Optional[int] = ...,
+        by: Optional[_Key] = ...,
+        get: Optional[Union[_Key, Sequence[_Key]]] = ...,
+        desc: bool = ...,
+        alpha: bool = ...,
+        store: Optional[_Key] = ...,
+        groups: bool = ...,
+    ) -> Pipeline: ...


Redis().sort() is overloaded to provide an accurate return type, but for Pipeline the return type is always Pipeline, so I removed the overloads.

typeshed/stubs/redis/redis/client.pyi

Lines 495 to 534 in e2fd852

@overload

def sort(

self,

name: _Key,

start: Optional[int] = ...,

num: Optional[int] = ...,

by: Optional[_Key] = ...,

get: Optional[Union[_Key, Sequence[_Key]]] = ...,

desc: bool = ...,

alpha: bool = ...,

store: None = ...,

groups: bool = ...,

) -> List[_StrType]: ...

@overload

def sort(

self,

name: _Key,

start: Optional[int] = ...,

num: Optional[int] = ...,

by: Optional[_Key] = ...,

get: Optional[Union[_Key, Sequence[_Key]]] = ...,

desc: bool = ...,

alpha: bool = ...,

*,

store: _Key,

groups: bool = ...,

) -> int: ...

@overload

def sort(

self,

name: _Key,

start: Optional[int],

num: Optional[int],

by: Optional[_Key],

get: Optional[Union[_Key, Sequence[_Key]]],

desc: bool,

alpha: bool,

store: _Key,

groups: bool = ...,

) -> int: ...

chdsbd · 2021-01-30T03:35:16Z

+    def smembers(self, name: _Key) -> Pipeline: ...
+    def smove(self, src, dst, value) -> Pipeline: ...
+    def spop(self, name, count: Optional[int] = ...) -> Pipeline: ...
+    def srandmember(self, name: _Key, number: Optional[int] = ...) -> Pipeline: ...


I removed the overload here too for the same reason.

typeshed/stubs/redis/redis/client.pyi

Lines 555 to 558 in e2fd852

@overload

def srandmember(self, name: _Key, number: None = ...) -> Optional[_Value]: ...

@overload

def srandmember(self, name: _Key, number: int) -> List[_Value]: ...

chdsbd · 2021-01-30T03:36:00Z

+        groups: bool = ...,
+    ) -> Pipeline: ...
+    def scan(self, cursor: int = ..., match: Optional[_Key] = ..., count: Optional[int] = ...) -> Pipeline: ...
+    def scan_iter(self, match: Optional[Text] = ..., count: Optional[int] = ...) -> Iterator[Any]: ...


The scan_iter and other _iter methods return generators that explode when next() is called on them.

chdsbd · 2021-01-30T03:37:48Z

+    def script_flush(self) -> Pipeline: ...
+    def script_kill(self) -> Pipeline: ...
+    def script_load(self, script) -> Pipeline: ...
+    def register_script(self, script: Union[Text, _StrType]) -> Script: ...


unlike other methods, this one actually returns Script

chdsbd · 2021-01-30T03:38:10Z

+    def pubsub_numpat(self) -> Pipeline: ...
+    def monitor(self) -> Monitor: ...
+    def cluster(self, cluster_arg: str, *args: Any) -> Pipeline: ...
+    def client(self) -> Any: ...


this method explodes when called

chdsbd · 2021-01-30T03:38:28Z

+    def pubsub_channels(self, pattern: _Key = ...) -> Pipeline: ...
+    def pubsub_numsub(self, *args: _Key) -> Pipeline: ...
+    def pubsub_numpat(self) -> Pipeline: ...
+    def monitor(self) -> Monitor: ...


This also returns Monitor instead of Pipeline

chdsbd · 2021-01-30T03:39:11Z

+        xx: bool = ...,
+        keepttl: bool = ...,
+    ) -> Pipeline: ...
+    def __setitem__(self, name, value) -> None: ...


should all the dunder methods be organized at the top?

Personally, I try to use the order the implementation uses. But it is your choice.

chdsbd · 2021-01-30T03:41:05Z

+    def object(self, infotype, key) -> Pipeline: ...
+    def ping(self) -> Pipeline: ...
+    def save(self) -> Pipeline: ...
+    def sentinel(self, *args) -> None: ...


this deprecated method doesn't do or return anything: https://github.com/andymccurdy/redis-py/blob/1a41cfd95a53a95b078084d8627be6b6fba3bb71/redis/client.py#L1448-L1451

srittau

Thank you for this PR and sorry for the long delay. Only one issue below.

srittau · 2021-02-22T11:57:09Z

    def ping(self, message: Optional[_Value] = ...) -> None: ...

-class BasePipeline:
+class Pipeline:


This devices from Redis in the implementation, so it should do so here as well. This will probably need # type: ignores where the signature of Pipeline does not match that of Redis. (But since Pipeline is a subtype of Redis that shouldn't be much of a concern.)

srittau · 2021-02-22T12:00:49Z

+        xx: bool = ...,
+        keepttl: bool = ...,
+    ) -> Pipeline: ...
+    def __setitem__(self, name, value) -> None: ...


Personally, I try to use the order the implementation uses. But it is your choice.

chdsbd · 2021-02-23T00:42:54Z

I updated Pipeline to inherit from Redis and added # type: ignore [override] to every method that returns Pipeline. Not all of these ignore statements are necessary because some methods in parent class are missing return types.

The ordering of methods should match the redis-py implementation. The Pipeline defines a few methods but inherits most methods from the Redis client.

Akuli · 2021-02-23T09:42:23Z

I think # type: ignore[override] is a non-standard mypy thing? This PR however uses # type: ignore [override], with a space. Maybe type checkers understand that to be same as just # type: ignore?

chdsbd changed the title ~~fix pipeline return types~~ [redis] fix pipeline return types Jan 30, 2021

chdsbd commented Jan 30, 2021

View reviewed changes

Merge branch 'master' into chris/redis-pipeline-types

3c99b3f

srittau requested changes Feb 22, 2021

View reviewed changes

chdsbd added 5 commits February 22, 2021 19:17

match redis ordering

d015b00

ignore overrides

2527c7c

Merge branch 'master' into chris/redis-pipeline-types

6272358

remove extra ignore

f381ea7

.

2761239

chdsbd requested a review from srittau February 23, 2021 00:42

srittau approved these changes Feb 23, 2021

View reviewed changes

srittau merged commit 46ac6bf into python:master Feb 23, 2021

brianmaissy mentioned this pull request Sep 12, 2021

[redis] pipeline return types (again) #6028

Closed

	@overload
	def sort(
	self,
	name: _Key,
	start: Optional[int] = ...,
	num: Optional[int] = ...,
	by: Optional[_Key] = ...,
	get: Optional[Union[_Key, Sequence[_Key]]] = ...,
	desc: bool = ...,
	alpha: bool = ...,
	store: None = ...,
	groups: bool = ...,
	) -> List[_StrType]: ...
	@overload
	def sort(
	self,
	name: _Key,
	start: Optional[int] = ...,
	num: Optional[int] = ...,
	by: Optional[_Key] = ...,
	get: Optional[Union[_Key, Sequence[_Key]]] = ...,
	desc: bool = ...,
	alpha: bool = ...,
	*,
	store: _Key,
	groups: bool = ...,
	) -> int: ...
	@overload
	def sort(
	self,
	name: _Key,
	start: Optional[int],
	num: Optional[int],
	by: Optional[_Key],
	get: Optional[Union[_Key, Sequence[_Key]]],
	desc: bool,
	alpha: bool,
	store: _Key,
	groups: bool = ...,
	) -> int: ...

	@overload
	def srandmember(self, name: _Key, number: None = ...) -> Optional[_Value]: ...
	@overload
	def srandmember(self, name: _Key, number: int) -> List[_Value]: ...

Uh oh!

Uh oh!

Conversation

chdsbd commented Jan 30, 2021

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!

chdsbd Jan 30, 2021 • 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!

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!

srittau 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!

chdsbd commented Feb 23, 2021

Uh oh!

Akuli commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chdsbd Jan 30, 2021 •

edited

Loading