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

Type redis.commands.base #8869

Open
HassanAbouelela opened this issue Oct 7, 2022 · 2 comments
Open

Type redis.commands.base #8869

HassanAbouelela opened this issue Oct 7, 2022 · 2 comments

Comments

@HassanAbouelela
Copy link
Contributor

HassanAbouelela commented Oct 7, 2022

Running stubtest on redis.commands.base reveals ~31 missing annotations, and from my experience I know some of the included functions are missing annotations, for example:

def getrange(self, key, start, end): ...

This file was chosen specifically because it includes a very large portion of the user interface, and operations users might want, thus giving the largest coverage for the stub.

I've already started work on implementing this, and will continue if approved. Does anyone have hints on how I could go about finding incomplete typehints like the one linked above? They don't seem to be properly marked.

@HassanAbouelela
Copy link
Contributor Author

HassanAbouelela commented Oct 7, 2022

I've done some more digging into the redis types, and found that the async classes (at least in core) are literally just the sync classes, but titled async. The difference is in their return type. The way this is annotated in the project is by defining every return type as T | Awaitable[T], in both the sync and async classes. Typeshed unpacks this by defining just the T in the sync class, and just the Awaitable[T] in the async one.

One thing we could do to reduce the implementation burden on typeshed is to use generics for this. It might seem a little insane at first, but the arguments are identical for both instances, and the return types are very similar in most cases. I've converted the SortedSetCommands class using this method to get a rough estimate, and we'd only need 9 TypeVars to accurately document the full class. Sample:

from typing import TypeVar, Awaitable, Generic

_IntReturn = TypeVar("_IntReturn", bound=int | Awaitable[int])
_FloatReturn = TypeVar("_FloatReturn", bound=float | Awaitable[float])
_FloatReturnOptional = TypeVar("_FloatReturnOptional", bound=float | None | Awaitable[float | None])
class _BaseSortedSetCommands(Generic[_IntReturn, _FloatReturn, _FloatReturnOptional]):
    def method_1(self) -> _IntReturn: ...
    def method_2(self) -> _FloatReturn: ...
    def method_3(self) -> _FloatReturnOptional: ...

class SortedSetCommands(_BaseSortedSetCommands[int, float, float | None]): ...
class AsyncSortedSetCommands(_BaseSortedSetCommands[Awaitable[int], Awaitable[float], Awaitable[float | None]]): ...

sync_class = SortedSetCommands()
reveal_type(sync_class.method_1())
reveal_type(sync_class.method_2())
reveal_type(sync_class.method_3())

async_class = AsyncSortedSetCommands()
reveal_type(async_class.method_1())
reveal_type(async_class.method_2())
reveal_type(async_class.method_3())

(This is a valid code sample that can be run with mypy to prove that correct types are returned)

How would we feel about something like this? Implementation can always continue as normal with the current system as well.

P.S: Shoutout to the staff at python discord for helping me figure this out.

@HassanAbouelela
Copy link
Contributor Author

HassanAbouelela commented Oct 7, 2022

Of note: the project itself might eventually switch to a completely different scheme for defining the sync and async APIs, one which might be typed using generics, see redis/redis-py#2119 (comment)

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

No branches or pull requests

1 participant