API change suggestions

[guersam]

Goals

1. Convenience
2. Matching underlying Redis commands as possible
3. Consistency

   Suggestions

4. Separate different commands as independent case classes

   For users using case classes directly, it''d be better to giving own case class to each command.

5. Wrap nested commands with an intermediate object

   ScriptLoad and ScriptSave issues SCRIPT LOAD, SCRIPT SAVE for each. Reflecting this pattern, suggest following structure:

   object Script {
      case class Load()
      case class Save()
   }

6. Make EVAL always expect LIST and remove EvalMulti~ commands

   Having single case class for single command is less error-prone. As List has similar interface with Option's, it won't be that inconvenient.

7. GetType to Type to match underlying Redis command

   As lowercase type is reserved in Scala, how about providing both of escaped type for matching and tpe for convenience?

8. Require Seq for repeated arguments as default

   Current interface like value: String, values: String* is convenient when giving it directly, but not when providing Seq directly. Making Seq as default and providing auxiliary constructors would be a solution.

[debasishg]

Regarding (1) I was thinking that users will not use the Commands directly. They will use the APIs. I made the commands thinking they should be implementation artifacts, which we can tweak for optimization/enhancements. So long the APIs don't change the published contract remain the same.

If we think of Commands as implementation stuff, then do we need (2) right now ? And thinking that performance will ultimately matter, this looks to add a bit of overhead by creating more objects and classes.

+1 on 3, 4 and 5.

WDYT ?

[guersam]

I once had thought users won't use case classes, but some of them do as shown in #19. Being an Akka citizen, directly dealing with ActorRef would be better in some cases.

(2) is for synchronization with Redis command and readability. I'm not a JVM expert and don't know how much overhead will be caused by accessing nested classes, but those kinds of command are not used often so it might be negligible. It's just matter of taste, so if you don't like it I don't mind discarding it.

[debasishg]

Ok .. I got it .. :+1: on this .. Just a tad of a concern that the case classes will be more susceptible to change than the published APIs ..

Regarding (2) it's true the impact is not much and affects only a small no of commands. So :+1: ..

[guersam]

The unified score-range argument looks ugly as shown in #24. Because default (inclusive) behavior is intuitive enough, suggest (probably) the last changes:

1. One of these
   1. Change argument order from Double, Boolean, Double, Boolean to Double, Double, Boolean, Boolean - More convenient but different from corresponding Redis API a bit
   2. Keep argument order as before, just add Double, Double as an alternative - looks better by itself but cannot have default limit argument in ~ByScore commands
2. Reverse min/max Inclusive to Exclusive, to match that the '(' for exclusivity is added 'explicitly'