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

Add support for optional count arg in rpop #308

Merged
merged 6 commits into from
Sep 13, 2024
Merged

Conversation

EmilioCristalli
Copy link
Contributor

rpop accepts an optional count argument to indicate how many elements should be removed and returned from the list

See https://github.com/redis/redis-rb/blob/9938411bd44383b795e05df900abce4df66daaef/lib/redis/commands/lists.rb#L114

Also had to change the shared examples a little bit to be able to pass the arguments they use and make a more accurate expectation on the error.
I think the args_for_method is making an assumption when the arity < 0 and always using [1, 2] (+ the key), but that doesn't work in all cases. In particular, rpop now has arity -2 (because it has 1 required arg + 1 optional) so calling rpop(key, 1, 2) was causing an argument error instead of Redis::CommandError (which we expect because of the redis value not being a list).

At first I tried to change args_for_method but it made other tests fail. And i suspect it won't be possible to have a generic args generator only based on arity (because some methods for example accept *args but the logic requires 1 or 2 args)

That's why i thought it might be a good idea for each test that includes the shared example to indicate what the correct args to make a valid call should be, but let me know what you think!

@sds sds merged commit 9df4b4f into sds:main Sep 13, 2024
11 checks passed
@sds
Copy link
Owner

sds commented Sep 13, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants