Types

[Rafiot]

Pull Request check-list

• Do tests and lints pass with this change?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?

Description of change

[Rafiot]

Woops, I wanted to do a PR on my own repo, but mixed it up and did the PR here, sorry for that.

It is not ready for merging, but I guess as it is here now, feel free to comment.

(WIP for #84)

[mkmkme]

Sure. As I mentioned in the issue, I'd really like to see the types within the code and not in separate files.

Also please you --signoff when committing changes (we can fix that later with the commits that are already here)

[Rafiot]

I'll also squash the commits, it is a mess for no good reason right now.

[Rafiot]

Now a note that is possibly a bit of a problem: smembers is marked as returning a set, but it doesn't. It always returns a list.

That's the reason the tests are failing there : https://github.com/Rafiot/valkey-py/actions/runs/10808666835/job/29982264205?pr=1#step:6:300

But also, the test suite is very much not helping as it casts the list to a set when len > 1 (again, see test suite above).

Question: Should we cast the response of smembers to a set? Or change the type hints?

[aiven-sal]

Question: Should we cast the response of smembers to a set? Or change the type hints?

We should change the type hints. There is some difference between RESP3 sets and Python's sets and it is not possible to represents all RESP3 sets as sets in Python, so the functions has to return a list. Its up to the user to cast it to set when/if they know that is is safe to do so.

Previous versions of the code returned a set, this is probably why the type hints are wrong.

[Rafiot]

smsmbers is now marked as returning a list

[mkmkme]

smsmbers is now marked as returning a list

@Rafiot I noticed the last commit changes some test code as well. That's a bit odd. Is your branch based on some old commit in valkey-py? Or is there other reason to change the test?

In my understanding, this PR should not change any behaviour of the library, it only adds the type hints.

[Rafiot]

@mkmkme I'm using the test suite as a way to validate the type hints, so to do that, I need to add type hints to the test suite, which explains the changes. I'll review it, but it should not change the logic of the tests just add the hints in a way mypy is happy. It seemed to me to be the best way to make sure the type hints are in-line with the current codebase.
Would you have another approach?

[mkmkme]

@Rafiot no-no, this approach sounds correct. I was just surprised by the commit 2e504dd

[Rafiot]

Oh, true. So I'm confused how the tests as they are now can pass, because the response is definitely a list, not a set:

>>> v = Valkey()
>>> v.ping()
True
>>> v.sadd('foo', 'bar')
1
>>> v.smembers('foo')
[b'bar']
>>> v.smembers('blah')
[]
>>> v.sadd('foo', 'baz')
1
>>> v.smembers('foo')
[b'bar', b'baz']

[Rafiot]

I just want to quickly update you on that one: I haven't forgotten, and will get back to it asap. This month got busy but I should be able to get back to this PR in a few days.