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

adds support for list lpos command #2229

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

kenus34
Copy link
Contributor

@kenus34 kenus34 commented Aug 20, 2020

This commit adds support for the lpos command added in Redis 6.0.6

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

This is incomplete. COUNT can used alone. But it doesn't support that. There's also MAXLEN option.

Params approach is better fit for this command.

@kenus34
Copy link
Contributor Author

kenus34 commented Aug 21, 2020

@sazzad16 Got it, I will do the Params approach.
also lpos returns a list of indexes when used with COUNT and a single index without COUNT so do you think making two sets of commands lpos and lposWithCount would make more sense in this case?
Splitting them would give us two sets of commands one set with return type long(lpos) and others with return type list(lposWithCount)

@sazzad16
Copy link
Collaborator

@kenus34 Yes. But you don't have to name lposWithCount. You may have following 2/3 methods:

  • Long lpos(key, element) *[optional]
  • Long lpos(key, element, lposParams)
  • Lis<Long> lpos(key, element, lposParams, long count)

@kenus34 kenus34 force-pushed the add-list-lpos-support branch 2 times, most recently from be39069 to 87a3720 Compare August 23, 2020 17:05
@kenus34 kenus34 force-pushed the add-list-lpos-support branch from 87a3720 to 450d4fb Compare August 23, 2020 17:23
@kenus34
Copy link
Contributor Author

kenus34 commented Aug 23, 2020

@sazzad16 Thanks a lot. I have made the changes and pushed the same.

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Almost done!

@kenus34
Copy link
Contributor Author

kenus34 commented Aug 24, 2020

Thanks for the suggestion. I have resolved the comments.

@sazzad16 sazzad16 requested a review from gkorland August 25, 2020 04:53
@sazzad16 sazzad16 added this to the 3.4.0 milestone Aug 25, 2020
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.

3 participants