Python: Adds Sort command #1439
Conversation
There was a problem hiding this comment.
What is CME/CMD?
Why do you split cluster and standalone implementations?
Why do you not include BY and GET pattern options for standalone?
You should also allow specifying route for cluster commands and document it.
Please, note, default routing and response handling isn't defined in redis-rs - this should be done prior to implementing a command.
| - ASC: Sort in ascending order. | ||
| - DESC: Sort in descending order. |
There was a problem hiding this comment.
Move this to ASC/DESC accordingly
There was a problem hiding this comment.
What is CME/CMD?
CMD: Cluster Mode Disable.
CME: Cluster Mode Enable.
As I wrote in the PR itself:
In CME, two arguments (BY/GET) aren't supported, so we dicided to create separate APIs for CMD and CME so in CME those 2 two won't appear as optional.
So for your questions:
Why do you split cluster and standalone implementations?
Because we want to limit this API call so the customer won't be able to send unsupported args to this function, so for ClusterClient we eliminated those BT\GET function's args.
Why do you not include BY and GET pattern options for standalone?
I do for standalone client, but not for ClusterClient, just for the reason I described.
You should also allow specifying route for cluster commands and document it.
This command operate on a specific key which like any other commands it doesn't need to get the route as an argument.
There was a problem hiding this comment.
Oh I see. The definition of CMD/CME answers to all of my questions.
Please, don't use it - it is not a Redis term. PR should be understandable for everyone.
Please, update PR description, and/or code comments. Add new function signatures to PR description for better explanation and examples.
I'll do another review round.
GilboaAWS
force-pushed
the
sort_python
branch
3 times, most recently
from
a06ee50
to
3173672
Compare
GilboaAWS
force-pushed
the
sort_python
branch
5 times, most recently
from
02eac42
to
75660c2
Compare
GilboaAWS
force-pushed
the
sort_python
branch
2 times, most recently
from
8fb41f5
to
eb38f12
Compare
There was a problem hiding this comment.
Please, add a TODO comment to update implementation to comply with Redis 8 and/or create a GH issue for that.
See also my nodes in #1439 (comment)
Co-authored-by: Shoham Elias <[email protected]>
Co-authored-by: Shoham Elias <[email protected]>
…ent arg to RedisClient instead of TRedisClient.
Co-authored-by: Aaron <[email protected]>
Co-authored-by: Aaron <[email protected]>
Co-authored-by: Aaron <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
* Adds sort command to python Co-authored-by: Yury-Fridlyand Co-authored-by: Shoham Elias Co-authored-by: Aaron Co-authored-by: ikolomi
* Adds sort command to python Co-authored-by: Yury-Fridlyand Co-authored-by: Shoham Elias Co-authored-by: Aaron Co-authored-by: ikolomi
We separated it into sort and sort_store.
In CME, two arguments (BY/GET) aren't supported, so we dicided to create separate APIs for CMD and CME so in CME those 2 two won't appear as optional.