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

Support ZRANK and ZREVRANK: Added the optional WITHSCORE argument #2489

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

codrin-ch
Copy link
Contributor

Description

Describe your pull request here


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

fix #2463 and #2464

@codrin-ch codrin-ch marked this pull request as ready for review April 29, 2023 11:28
@leibale
Copy link
Collaborator

leibale commented Apr 29, 2023

@codrin-ch thanks for contributing!
The WITHSCORE argument should be implemented as a separate command since it has other return type (ZRANK_WITHSCORE and ZREVRANK_WITHSCORE). See ZDIFF_WITHSCORES for example.

@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.08% ⚠️

Comparison is base (a217cc1) 95.71% compared to head (25939e5) 95.64%.
Report is 1 commits behind head on master.

❗ Current head 25939e5 differs from pull request most recent head d2dbea6. Consider uploading reports for the commit d2dbea6 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2489      +/-   ##
==========================================
- Coverage   95.71%   95.64%   -0.08%     
==========================================
  Files         458      460       +2     
  Lines        4531     4543      +12     
  Branches      506      506              
==========================================
+ Hits         4337     4345       +8     
- Misses        127      131       +4     
  Partials       67       67              
Files Changed Coverage Δ
packages/client/lib/commands/ZRANK_WITHSCORE.ts 80.00% <80.00%> (ø)
packages/client/lib/commands/ZREVRANK_WITHSCORE.ts 80.00% <80.00%> (ø)
packages/client/lib/cluster/commands.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@codrin-ch
Copy link
Contributor Author

I think the coverage missing is due to the redis version from the testing cluster.
The tests have the min version 7.2 and as a result I suspect that the tests are skipped for the new functions

@leibale
Copy link
Collaborator

leibale commented May 1, 2023

@codrin-ch this is exactly what is happening. I'll add Redis 7.2 to the tests matrix soon.

@leibale
Copy link
Collaborator

leibale commented May 24, 2023

@codrin-ch sorry for the long delay, but looks like the tests are failing (you can ignore the "ACL GETUSER" test, but:

  1. "client.zRankWithScore empty response"
  2. "client.zRankWithScore"
  3. "client.zRevRankWithScore empty response"
  4. "client.zRevRankWithScore"
    are all failing...

https://github.com/redis/node-redis/actions/runs/5037516252/jobs/9034411168?pr=2489#step:7:2430

If you don't have time to work on it, I'll take it from here and finish it, just let me know.. :)

leibale added a commit to leibale/node-redis that referenced this pull request Jun 22, 2023
@TarSzator
Copy link

Can something be done to finish this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ZRANK: Added the optional WITHSCORE argument.
4 participants