-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
refactoring set command to use SetParams for the optional parameters #878
Conversation
I think that checking arguments is not necessary and actually wrong. |
@xetorthio in my opinion, Redis should also complain about it. If we set both, EX and PX, what expire time should be used? And if we set both EX and NX, no value will be returned. We know that those values are wrong, so why don't warning the user about it? It can avoid wrong use of the parameters. Actually, I'll ask this in Redis repo too. |
Actually, after we talked, I agree with you! I did remove the checking. |
So LGTM! |
Thanks @xetorthio. @HeartSaVioR and @marcosnils what do you think about it? |
public void set(final byte[] key, final byte[] value, final byte[] nxxx, final byte[] expx, | ||
final long time) { | ||
sendCommand(Command.SET, key, value, nxxx, expx, toByteArray(time)); | ||
public void set(final byte[] key, final byte[] value, final byte[] nxxx, final byte[] expx, final byte[] time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nykolaslima
Even BinaryClient uses ZParams, so we can change its signature to public void set(final byte[] key, final byte[] value, final SetParams params).
Same things can be applied to BinaryJedis.
@xetorthio @marcosnils What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that.
This way we will be more flexible if *Command*Parameters
have any changes. I'll fix it.
@nykolaslima |
@nykolaslima any updates about this? |
@marcosnils and @HeartSaVioR sorry about my delay, I'm really busy this week but I'll try to finish it today! |
@nykolaslima PING |
Conflicts: src/main/java/redis/clients/jedis/Jedis.java src/main/java/redis/clients/jedis/JedisCluster.java
@HeartSaVioR @marcosnils @xetorthio Guys, I'm sorry for taking so long on this. I was very busy during these days. Please, take a look at the PR. Now we got all tests passing. If you have any considerations, please let me know. By the way, congratulations on the new versions releases 🎉 |
@nykolaslima Well done! When other collaborators take a look and OK then I'll merge. |
I'm +1, and since any collaborators didn't review this such a long time, it would be good to merge now. |
@nykolaslima Thanks for the great effort! I merged this to master. |
👍 |
Thank you guys. And i'm sorry for being so away those times.. |
@nykolaslima Not at all! We have many issues to resolve, and I'd be happy if you lend a hand. :) |
It closes #877
binary
layer receiving all the possible parameters. Thebinary
layer was receiving expire time inlong
instead ofbyte[]
, so I also change it.set
methods that receive only a few optional parameters.SetParams
.