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.
@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.
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
binarylayer receiving all the possible parameters. Thebinarylayer was receiving expire time inlonginstead ofbyte[], so I also change it.setmethods that receive only a few optional parameters.SetParams.