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 keys to evalscript #109

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Add keys to evalscript #109

merged 3 commits into from
Jul 12, 2024

Conversation

aviks
Copy link
Member

@aviks aviks commented Jul 5, 2024

evalscript should take a keys argument, according to https://redis.io/docs/latest/commands/eval/

@aviks
Copy link
Member Author

aviks commented Jul 5, 2024

The existing code/tests worked by concatenating keys and args, but looking at the redis docs, as well as the ruby driver, I would have expected that keys and args would be two separate arguments.

@aviks aviks requested a review from tanmaykm July 5, 2024 21:39
@tanmaykm
Copy link
Member

tanmaykm commented Jul 6, 2024

Yes, this looks like a valid fix to evalscript.

You probably meant the PR title to say evalscript instead of evalsha? Because evalsha already seems to be accepting keys and args separately.

Also wondering if we should use @redisfunction for evalscript.

@aviks aviks changed the title Add keys to evalsha Add keys to evalscript Jul 6, 2024
@aviks
Copy link
Member Author

aviks commented Jul 6, 2024

I realised this would be breaking unless we support the old behaviour as well.

@tanmaykm
Copy link
Member

tanmaykm commented Jul 7, 2024

I realised this would be breaking unless we support the old behaviour as well.

True. Maybe we add a @redisfunction for evalscript and have the old method signature mapped to it with @deprecate?

@tanmaykm
Copy link
Member

tanmaykm commented Jul 7, 2024

Maybe we add a @redisfunction for evalscript

On second thoughts, maybe we shouldn't do @redisfunction for that because it then needs to be named eval to match the redis command, and it will clash with Base.eval.

@tanmaykm
Copy link
Member

I guess we can merge this and go for a major version bump as discussed in #110 (comment) ? Maybe I'll merge this in a bit.

I also think we should allow evalscript in PipelineConnection and TransactionConnection. Will add a separate PR for that.

Shouldn't `evalscript` take a `keys` argument, at least according to https://redis.io/docs/latest/commands/eval/ ?
@tanmaykm tanmaykm merged commit 085dca5 into master Jul 12, 2024
3 checks passed
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