Skip to content

Type redis.commands.base #8869

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

Closed
HassanAbouelela opened this issue Oct 7, 2022 · 9 comments
Closed

Type redis.commands.base #8869

HassanAbouelela opened this issue Oct 7, 2022 · 9 comments
Labels
stubs: incomplete Annotations or sub-modules missing from an existing package or module topic: redis Issues related to the redis third-party stubs

Comments

@HassanAbouelela
Copy link
Contributor

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

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)

@AlexWaygood AlexWaygood added the stubs: incomplete Annotations or sub-modules missing from an existing package or module label Apr 29, 2023
@AlexWaygood AlexWaygood added the topic: redis Issues related to the redis third-party stubs label Jul 8, 2023
@zulrang
Copy link

zulrang commented Jan 11, 2024

Is there any way to appropriately satisfy type checkers when using these Union'd methods?

error: Incompatible types in "await" (actual type "Awaitable[set[Any]] | set[Any]", expected type "Awaitable[Any]")  [misc]

@HassanAbouelela
Copy link
Contributor Author

@zulrang is that with the proposed system, or the current typings? Could you include a code sample.

@zulrang
Copy link

zulrang commented Jan 13, 2024

@zulrang is that with the proposed system, or the current typings? Could you include a code sample.

Current typings.

Example - this fails mypy:

await redis_client.smembers(key)

As does any async call to core commands in redis/commands/core.py which are assigned copies (AsyncListCommands = ListCommands) and not independent classes.

@srittau
Copy link
Collaborator

srittau commented Jan 14, 2024

Sorry for not answering to this issue before: Any improvements to the stubs are welcome! While the redis stubs have been improved over the last few years, there are still many untyped corners. Introducing generics after the fact is always a bit problematic, unless we get something like PEP 696 (defaults for type vars), since it can break users who already use a class as annotation. But we still do it if it provides a clear advantage.

That said, I'm going to close this issue – not because it's invalid, but because stubs being incomplete is normal in typeshed, and we don't keep track of that situation with issues. (Also, redis.commands.base doesn't exist in the current stubs.)

@srittau srittau closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2024
@zulrang
Copy link

zulrang commented Jan 14, 2024

The issue here isn't incompleteness, it's straight-up invalid types. Union[Awaitable(type), type] doesn't work at all for any type checker. A function cannot be used in both sync and async contexts like that.

The issues are found in the latest versions in redis.commands.core

@HassanAbouelela
Copy link
Contributor Author

Is that still the case? I'm looking at main, and the current async types are properly typed out (instead of being unions). Here is the currently published version.

Perhaps the confusion is caused by the upstream package type, which are indeed unions as you've described, but installing types-redis should fix that.

@zulrang
Copy link

zulrang commented Jan 15, 2024

Is that still the case? I'm looking at main, and the current async types are properly typed out (instead of being unions). Here is the currently published version.

Perhaps the confusion is caused by the upstream package type, which are indeed unions as you've described, but installing types-redis should fix that.

And there was the answer. It was my confusion. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: incomplete Annotations or sub-modules missing from an existing package or module topic: redis Issues related to the redis third-party stubs
Projects
None yet
Development

No branches or pull requests

4 participants