Skip to content

zrange, zrangestore and zrevrange options #1617

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

Closed
wants to merge 4 commits into from

Conversation

Pigrenok
Copy link

@Pigrenok Pigrenok commented Oct 15, 2021

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • 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)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Two small changes added to the code:

  1. Added capability to return raw server output (if not error returned) for unknown command. It will allow more flexibility for people to use in projects with more custom requirements (e.g. to use redis modules).
  2. Added byScore, byLex and limit options to zrange, zrangestore and zrevrange in line with Redis documentation. If for zrange there are alternatives zrangebyscore and zrangebylex, for zrangestore there is no alias and it is extremely useful to have these capabilities to implementing secondary structures and operation with them on the server side.

All tests run successfully, but I have not added tests for these new options. I enabled CI in my fork but It has not run yet. The functions docstrings were updated accordingly.

This is my first public PR, so, please, do not judge too strictly. If I did something completely wrong, please, forgive me :)

@Pigrenok
Copy link
Author

Sorry, I realised I did not push the changes after flake8 test. I will push it tomorrow and rerun CI test.

@Pigrenok
Copy link
Author

Again, sorry for failing CI test before. Now everything is passed.

@chayim
Copy link
Contributor

chayim commented Oct 18, 2021

@Pigrenok Thank you for the contribution, keep them coming!

Unfortunately this is duplicate effort. #1603 was sent for PR a few days back, and we'll be merging in that implementation.

But there are plenty of issues that we need help with! Maybe you'd be interested in tackling something else? I'm more than happy to point out areas we could use help with!

@chayim chayim closed this Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants