Skip to content

Add withscore option to ZRANK and ZREVRANK. #11235

Merged
oranagra merged 6 commits intoredis:unstablefrom
CharlesChen888:feature-zrankwithscore
Nov 28, 2022
Merged

Add withscore option to ZRANK and ZREVRANK. #11235
oranagra merged 6 commits intoredis:unstablefrom
CharlesChen888:feature-zrankwithscore

Conversation

@CharlesChen888
Copy link
Contributor

@CharlesChen888 CharlesChen888 commented Sep 5, 2022

Add an option "withscore" to ZRANK and ZREVRANK.

Add [withscore] option to both zrank and zrevrank, like this:

z[rev]rank key member [withscore]

discussion

To close #11111
The conclusion in #11111 is adding 2 options [withscore] and [rev] to zrank like this:

zrank key member [withscore] [rev]

However, since we already have zrevrank and it shares the same code with zrank, it would be quite inconvenient to add option [rev] to zrank, as this requires some ugly judgements in the code. Also it breaks the symmetry between zrank and zrevrank, in the aspects of both code implementation and command calling.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

It seems to be in line with the discussed solution. 👍

How about adding a simple test case for both commands?

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

@soloestoy
Copy link
Contributor

good job! welcome to redis : )

@soloestoy soloestoy changed the title Add withscores option to ZRANK and ZREVRANK. Add withscore option to ZRANK and ZREVRANK. Sep 6, 2022
@zuiderkwast zuiderkwast added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Sep 8, 2022
@enjoy-binbin enjoy-binbin added state:needs-doc-pr requires a PR to redis-doc repository release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged labels Sep 8, 2022
@oranagra oranagra self-assigned this Nov 9, 2022
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

sorry for the delay (been busy and forgot).
mostly LGTM, i need some reassurance on the response type when items is missing.

@redis/core-team i don't recall if we conceptually approved this one or not, PTAL.

p.s. a redis-doc PR is needed too.

@oranagra
Copy link
Member

conceptually approved in a core-team meeting.

@oranagra oranagra merged commit eeca7f2 into redis:unstable Nov 28, 2022
@oranagra
Copy link
Member

@CharlesChen888 sorry it took so long.
can you please make a PR for https://github.com/redis/redis-doc/pulls

zuiderkwast pushed a commit to redis/redis-doc that referenced this pull request Jan 24, 2023
For PR redis/redis#11235, which adds the WITHSCORE option to command ZRANK and ZREVRANK.
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
Add an option "withscores" to ZRANK and ZREVRANK.

Add `[withscore]` option to both `zrank` and `zrevrank`, like this:
```
z[rev]rank key member [withscore]
```
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
Add an option "withscores" to ZRANK and ZREVRANK.

Add `[withscore]` option to both `zrank` and `zrevrank`, like this:
```
z[rev]rank key member [withscore]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:needs-doc-pr requires a PR to redis-doc repository state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

Status: Done
Archived in project

Development

Successfully merging this pull request may close these issues.

[NEW] zrank with score option

6 participants