update the return type of scan family

[amirreza8002]

hi
just did a few methods to see how it goes
i plan to continue the work, so if you have a feedback i'm happy to hear.

one thing i notice is that a lot of valkey method's return types include Awaitable, which is plain wrong and very annoying, so hoepfully you'll see more of me on this :)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.19%. Comparing base (4f7e880) to head (9adb262).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   76.19%   76.19%           
=======================================
  Files         130      130           
  Lines       33907    33907           
=======================================
  Hits        25837    25837           
  Misses       8070     8070           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mkmkme]

Hey @amirreza8002 !

Thanks a lot for your contribution! Do I understand correctly that the return type was wrong because this part of code is not in fact async? :) Or is it something else I'm missing?

[amirreza8002]

hi
no
the old type was Union[Awaitable[Any], Any]
the new type is specific, and Awaitable is wrong for sync and async alike.

also, to my knowledge, none of the methods in valkey-py, sync or async, return an awaitable
https://docs.python.org/3/library/typing.html#annotating-callable-objects

also typing.Awaitable is deprecated

[mkmkme]

the old type was Union[Awaitable[Any], Any]
the new type is specific

Could you expand this thought please?

https://docs.python.org/3/library/typing.html#annotating-callable-objects

Yeah, it seems like typing.Awaitable[] would be appropriate only if this function returned a callback?

Anyway, this piece of PEP 484 says we should not mark the return type as Awaitable here:

Coroutines introduced in PEP 492 are annotated with the same syntax as ordinary functions. However, the return type annotation corresponds to the type of await expression, not to the coroutine type:

async def spam(ignored: int) -> str:
    return 'spam'

async def foo() -> None:
    bar = await spam(42)  # type: str

also typing.Awaitable is deprecated

What I'm reading from this document and PEP 585, it's only plain typing.Awaitable that is deprecated. It has become a generic now, which is used in ResponceT.

---

Also, I'm a bit concerned that Union[] has been replaced with a Tuple[] here. It is probably correct, but I will have to double-check it tomorrow.

Sorry for being picky here, I'm just not that experienced with type hints in Python so wanna make sure we're not breaking anything here :)

[amirreza8002]

hi
so i ran a bunch of test queries and from what i can tell these methods only return a tuple, with the values I've written.

meaning the first element of the tuple is an integer (named cursor), and the second element is either a list or a dict depending on the method

to test this i simply put some sample data in the database, queried it with these methods, and printed the type if the values returned.

the Awaitable type hint, from what i understand and my IDE tells me, is for when the value it self is in fact awaitable, meaning it returns a callable

I'm not a type hint guy myself so be as picky as you can :)

if needed I'll post some sample codes here tomorrow to demonstrate

[amirreza8002]

on another note
if you are planning to drop python 3.8 soon I should rewrite these type hints to use 3.9+ syntax

let me know if that's the case

[mkmkme]

@amirreza8002

if you are planning to drop python 3.8 soon I should rewrite these type hints to use 3.9+ syntax

Could you please tell me what type hints improvements did you have in mind particularly?

[amirreza8002]

imports like typing.Tuple, typung.Set, etc... are for 3.8-

we can just use tuple, set... from the built-ins

not that important but avoids some importing

[amirreza8002]

another thought

since it'll take time to fix the types of this package, is it possible to create a new branch at valkey-py and move this work and other people's work related to types there?

[mkmkme]

I'm actually thinking that perhaps the best idea would be to start applying your changes with smaller chunks. A single PR (a single commit, even!) affecting 17 thousands lines of code sounds scary and will take significant time to review and test.
We could create a separate epic for this and a bunch of issues to be fixes with a bunch of PRs.

Do you think that would be feasible? And what's your opinion on that?

[amirreza8002]

some basic exapmels for this PR:

scan:

>>> from valkey import Valkey
>>> v = Valkey()
>>> v.set("foo", "bar")
True
>>> v.scan(0)
(0, [b'foo'])
>>> c, r = v.scan(0)
>>> type(c)
<class 'int'>
>>> type(r)
<class 'list'>

sscan:

>>> v.sadd("myset", "foo", "bar")
2
>>> v.sscan("myset")
(0, [b'foo', b'bar'])
>>> c, r = v.sscan("myset")
>>> type(c)
<class 'int'>
>>> type(r)
<class 'list'>

hscan:

>>> v.hset("myhash", "hash1", "value")
1
>>> v.hscan("myhash")
(0, {b'hash1': b'value'})
>>> c, r = v.hscan("myhash")
>>> type(c)
<class 'int'>
>>> type(r)
<class 'dict'>

zscan:

>>> v.zadd("thisset", {"a": 1, "b": 4})
2
>>> v.zscan("thisset")
(0, [(b'a', 1.0), (b'b', 4.0)])
>>> c, r = v.zscan("thisset")
>>> type(c)
<class 'int'>
>>> type(r)
<class 'list'>

[amirreza8002]

I'm actually thinking that perhaps the best idea would be to start applying your changes with smaller chunks. A single PR (a single commit, even!) affecting 17 thousands lines of code sounds scary and will take significant time to review and test. We could create a separate epic for this and a bunch of issues to be fixes with a bunch of PRs.

Do you think that would be feasible? And what's your opinion on that?

mm
i agree that small changes are better
but i don't know what is the best way to move it forward, whatever you decide is fine by me

[mkmkme]

Maybe add it file-by-file? Or in larger files like core.py even class-by-class?
We can also add mypy to the CI right away but make the check non-fatal for now. Or make it scan only the files we already reworked.

[aiven-sal]

I'm actually thinking that perhaps the best idea would be to start applying your changes with smaller chunks. A single PR (a single commit, even!) affecting 17 thousands lines of code sounds scary and will take significant time to review and test. We could create a separate epic for this and a bunch of issues to be fixes with a bunch of PRs.
Do you think that would be feasible? And what's your opinion on that?

mm i agree that small changes are better but i don't know what is the best way to move it forward, whatever you decide is fine by me

It is fine to fix small chunks of typing issues one by one over time or work on a huge draft PR that fix everything at once everything is fixed (e.g. you have mypy enabled and it's all green) it can be split in smaller PRs for easier review.
It doesn't really matter to me how the development happens, as long as the people involved in it are happy with it.
But it's difficult for a single 20k-changes PR to be accepted.

[mkmkme]

some basic exapmels for this PR

Yeah, thanks! This PR looks good and I'll merge it now. I'd like to continue the discussion regarding the types tho. We can continue it here or do it in Matrix: https://matrix.to/#/#valkey:matrix.org

[amirreza8002]

Maybe add it file-by-file? Or in larger files like core.py even class-by-class? We can also add mypy to the CI right away but make the check non-fatal for now. Or make it scan only the files we already reworked.

i'll probably go method by method, or a few methods per PR
i'm refactoring django-valkey at the moment and try to mirror the changes here as i go

I'd like to continue the discussion regarding the types tho. We can continue it here or do it in Matrix: https://matrix.to/#/#valkey:matrix.org

i don't have matrix at the moment, i'll see if i can set it up