Being able to use #set with only expire time

[nykolaslima]

We currently have the method: String set(String key, String value, String nxxx, String expx, long time)

But if we want to only pass the ex or px timeout parameter, we are not able to because nx or xx parameter is required.

I'm thinking in two possible solutions:

1. create a new set method, with this signature: String set(String key, String value, String expx, long time)
2. accept null value in the current method, so we can call this way: jedis.set("myKey", "value", null, "ex", 60)

I believe that the 1. is the better solution, what do you guys think @HeartSaVioR @marcosnils ?

[HeartSaVioR]

I have another approach.
There're too many optional parameters, so we may have to change our approach and use new Param class like ZParam instead of method overriding.

[xetorthio]

Overloading would be backwards compatible, right?

On 23:11, Wed, Feb 4, 2015 Jungtaek Lim notifications@github.com wrote:

I have another approach.
There're too many optional parameters, so we may have to change our
approach and use new Param class like ZParam instead of method overriding.

—
Reply to this email directly or view it on GitHub
#877 (comment).

[nykolaslima]

@xetorthio overloading will be backwards compatible but it's not maintainable. We will need to have big methods with all optional parameters or many methods with all possible combinations.

I suggest us to create a Params class, where users can build the optional parameters:

public abstract class Params {
  protected Map<String, String> params;
}

For the SET command, we could have a SetParams:

public class SetParams extends Params {
  public SetParams ex(int expire) {
    params.put("ex", expire);
  }

  public SetParams px(long expire) {
    params.put("px", expire);
  }
}

This way we could call set method like this:
jedis.set("key", "value", new SetParams().px(1000).nx())

If you guys approve it, we could use this pattern for all commands with optional parameters (maybe for 3.0?)

@xetorthio @marcosnils @HeartSaVioR

[HeartSaVioR]

Yes, I'm aiming it for 3.0.0, fit for breaking change.

[xetorthio]

Ok, although overloads are very convenient too.I don't know if I would have
a strict rule about this.

On Thu Feb 05 2015 at 11:14:55 AM Jungtaek Lim notifications@github.com
wrote:

Yes, I'm aiming it for 3.0.0, fit for breaking change.

—
Reply to this email directly or view it on GitHub
#877 (comment).

[nykolaslima]

@xetorthio so what approach should we use?

1. overloads
2. Params class

@HeartSaVioR @marcosnils

[xetorthio]

I think both... but we need some rules to decide when to use each approach.

On Thu Feb 05 2015 at 12:54:02 PM Nykolas Laurentino de Lima <
notifications@github.com> wrote:

@xetorthio https://github.com/xetorthio so what approach should we use?

1. overloads
2. Params class

@HeartSaVioR https://github.com/HeartSaVioR @marcosnils
https://github.com/marcosnils

—
Reply to this email directly or view it on GitHub
#877 (comment).

[nykolaslima]

IMHO, we should keep Params class for every optional parameters.

We could use overload when command got just one optional parameter, but doing this, if the command evolve in Redis and get more optional parameters it will be more difficult to update Jedis, because we will need to add other method or change the method signature to support it.

Using the Params class approach, we could have the same method signature and and just add a new method into *Command*Params class. This way we will always be backwards compatible and we can also quickly evolve following the latest Redis versions.

[xetorthio]

I understand the problem I just don't think that in order to do SET foo bar EX 10 I should do:

SetParams p = new SetParams();
p.ex = 10;
jedis.set("foo", "bar", p);

Instead of:

jedis.set("foo", "bar", 10);

There is a 3rd option that just came to my head, which is adding a single
overload for every command that admits optional params with a vararg of
strings. So basically the above command could be declared like this:

jedis.set("foo", "bar", "EX", "10");

This last one avoids all Param classes. We could potentially remove them
all together.

On Thu Feb 05 2015 at 1:03:25 PM Nykolas Laurentino de Lima <
notifications@github.com> wrote:

IMHO, we should keep Params class for every optional parameters.

We could use overload when command got just one optional parameter, but
doing this, if the command evolve in Redis and get more optional parameters
it will be more difficult to update Jedis, because we will need to add
other method or change the method signature to support it.

Using the Params class approach, we could have the same method signature
and and just add a new method into _Command_Params class. This way we
will always be backwards compatible and we can also quickly evolve
following the latest Redis versions.

—
Reply to this email directly or view it on GitHub
#877 (comment).

[nykolaslima]

We can create the *Command*Params class using the fluent builder pattern, so we avoid this setters and it could be a little easier:
jedis.set("foo", "bar", new SetParams().ex(10));

We can also create static imports and it will be even more short:

import static jedis.SetParams.setParams;

jedis.set("foo", "bar", setParams().ex(10));

I also thought it would be nice to receive a varargs, but the optional parameters are not always key/value pairs. For example, SET command could receive a NX or XX param, that isn't key/value param.
In this case, I think it would be a little awkward: jedis.set("foor", "bar", "ex", "10", "nx"). The user could also do things like: jedis.set("foo", "bar", "ex", "nx", 10).

I believe that having a *Command*Params class will be good because users can see what optional parameters they can use, it's like a documentation. When you create a SetParams you will see all the possible params, for the user perspective it's much easier than a varargs of objects.

[HeartSaVioR]

Using vargs can only work when we pass params to Redis directly, not caring what is key and what is value, and what is optional keyword.
It's no problem with simple commands, but it's too hard to use with complex commands including optional keywords.

Giving users to freedom is great, but it's not good anymore when it's near raw things, means users should know sequence of parameters, what keywords it needs.

[nykolaslima]

As discussed with @xetorthio, I believe that we all agree with Params class approach.

I'll implement it for this case and open a PR for you guys to review. Thanks!