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

Python: add ZDIFF command #1401

Merged
merged 3 commits into from
May 13, 2024

Conversation

aaron-congo
Copy link
Collaborator

Issue #, if available:
N/A

Description of changes:
https://redis.io/docs/latest/commands/zdiff/

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aaron-congo aaron-congo added the python Python wrapper label May 9, 2024
@aaron-congo aaron-congo requested a review from a team as a code owner May 9, 2024 23:42
await self._execute_command(RequestType.ZDiff, [str(len(keys))] + keys),
)

async def zdiff_withscores(self, keys: List[str]) -> Dict[str, float]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we use Dict and not Mapping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that Mapping was a thing until somewhat recently, the existing code sometimes uses Dict and sometimes Mapping. This Stack Overflow post seems to recommend Dict instead of Mapping if its a return value, but it seems like both still work fine and it sounds like you would prefer Mapping so I will switch

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure what we should choose, but whatever it is, it should be consistent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also see that Dict is deprecated, am I misunderstanding the post/ python documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently typing.Dict is deprecated in favor of builtins.dict (see here). Most commands use Mapping so I've switched to Mapping

python/python/glide/async_commands/core.py Show resolved Hide resolved
python/python/glide/async_commands/core.py Show resolved Hide resolved
python/python/glide/async_commands/core.py Outdated Show resolved Hide resolved
python/python/tests/test_async_client.py Show resolved Hide resolved
@aaron-congo
Copy link
Collaborator Author

@shohamazon Thanks for the review, all comments are addressed now

async def zdiff_withscores(self, keys: List[str]) -> Mapping[str, float]:
"""
Returns the difference between the first sorted set and all the successive sorted sets, with the associated scores.
When in Cluster mode, all keys must map to the same hash slot.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please add whitespace prior to this line

@acarbonetto acarbonetto merged commit 3af09e0 into valkey-io:main May 13, 2024
6 checks passed
@acarbonetto acarbonetto deleted the python/integ_acongo_zdiff branch May 13, 2024 23:31
avifenesh pushed a commit that referenced this pull request May 14, 2024
* Python: add ZDIFF command (#255)

* Add PR link in CHANGELOG

* PR suggestions
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
* Python: add ZDIFF command (#255)

* Add PR link in CHANGELOG

* PR suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Python wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants