-
Notifications
You must be signed in to change notification settings - Fork 65
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
Java: Add SETRANGE
command.
#1235
Java: Add SETRANGE
command.
#1235
Conversation
* Add `SETRANGE` command. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>
java/client/src/main/java/glide/api/commands/StringCommands.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/commands/StringCommands.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/commands/StringCommands.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]>
Will resolve conflicts later, after merging few more PRs pending |
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.
Do we not include changelogs for small core changes?
No |
java/client/src/main/java/glide/api/commands/StringCommands.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]>
…TRANGE Signed-off-by: Yury-Fridlyand <[email protected]>
* @param key The key of the string to update. | ||
* @param offset The position in the string where <code>value</code> should be written. | ||
* @param value The string written with <code>offset</code>. | ||
* @return The length of the string stored at <code>key</code> after it was modified. |
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.
If key
holds a value that is not a string, an error is returned.
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.
We do not document error cases for all commands to keep docs easy.
This comment is applicable to all commands - do you want to add it to a common place? readme?
java/client/src/main/java/glide/api/commands/StringBaseCommands.java
Outdated
Show resolved
Hide resolved
* @return The length of the string stored at <code>key</code> after it was modified. | ||
* @example | ||
* <pre>{@code | ||
* long len = client.setrange("key", 6, "GLIDE").get(); |
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.
Shouldn't it be Long?
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.
@acarbonetto
Please make sure that the examples compile
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.
Updated. Both are ok and compilable
int offset = 42; | ||
String str = "pewpew"; | ||
String[] arguments = new String[] {key, Integer.toString(offset), str}; | ||
Long value = 10L; |
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.
minor: value -> responseValue (value is confusing in the key-value context)
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.
There are 99 tests in this file and almost all of them have variable named "value".
I'm going to refactor tests in another PR (and rename the var). Is it OK for you?
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.
commandResult
or futureResult
is even less ambiguous.
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.
Approved - please make sure to fix the comments before merging
Signed-off-by: Yury-Fridlyand <[email protected]>
…TRANGE Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
* Add `SETRANGE` command. (#183) * Add `SETRANGE` command. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * Typo fix. Signed-off-by: Yury-Fridlyand <[email protected]> * Update UT. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>
* Add `SETRANGE` command. (#183) * Add `SETRANGE` command. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * Typo fix. Signed-off-by: Yury-Fridlyand <[email protected]> * Update UT. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>
Issue #, if available:
N/A
Description of changes:
Add
SETRANGE
command. https://redis.io/commands/setrange/By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.