Inconsistent key names in COPY / RENAME / RENAMEX / PUBLISH command documentation and other fixes. #2196
Conversation
…ocumentation and other documentation fixes.
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
All and all, I am fine with most of these changes. In general, I prefer self-describing code and effective Javadoc. Our Javadoc is in much need of improvement, as is apparent, even in the small collection of refactorings here.
If users have to guess or lookup what something means, then this is a failure of the framework since its causes a context switch in the users workflow and disrupts their productivity.
By way of example, if an @param tag reads:
@param key {@link String key} identifying and referring to the data structure or value in Redis, such as a String, Integer, dates, arrays or hashes; must not be {@literal null}`Vs.
@param key must not be {@literal null}.Then as user, I don't have to wonder what a Redis "key" is? I might still need to lookup the fact that Redis is a dictionary mapping keys to different types of values, but that is a different issue. This might sound trivial, but can be easily taken for granted.
Of course, one could argue that this will happen to new Redis users regardless, but minimizing this effect will go along way in increasing adoption of our frameworks due to ease of use and intuitiveness.
| * @param source must not be {@literal null}. | ||
| * @param destination must not be {@literal null}. |
There was a problem hiding this comment.
Now, neither source nor destination describe what they are. For instance, are they keys, are they values, what?
"must not be {@literal null}" is not informative and arguably no longer needed given the package-level @NonNullApi requires non-null values when method parameters are not explicitly annotated with @Nullable.
I am fine with shorter names as long as the Javadoc makes it apparent what things are and what they are used for.
| * @param newKey must not be {@literal null}. | ||
| * @return {@literal null} when used in pipeline / transaction. | ||
| * @see <a href="https://redis.io/commands/renamenx">Redis Documentation: RENAMENX</a> | ||
| */ | ||
| @Nullable | ||
| Boolean renameIfAbsent(K oldKey, K newKey); | ||
| Boolean renameIfAbsent(K key, K newKey); |
There was a problem hiding this comment.
I think the renames are fine here.
But, I think the Javadoc needs serious work. For example, what does the oldKey/key refer to? Is a key to a data structure in Redis? Is it a key in a hash data structure mapping. I know I was confused by this when I first started learning about Redis.
There was a problem hiding this comment.
It is not our responsibility to educate folks about the concepts of their data store.
jxblum
added
type: task
I tried to be conservative with the parameter name changes, only changing methods with the same function signature as in the Redis docs. I couldn't bring myself to change param 'dbIndex' to 'db' in RedisOperations.move(). I think our name is more descriptive.
The other changes are javadoc inconsistencies I found when reading through. Let me know if you need anything changed.