Skip to content
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

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

Yury-Fridlyand
Copy link
Collaborator

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.

* 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]>
@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Apr 4, 2024
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner April 4, 2024 21:26
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand
Copy link
Collaborator Author

Will resolve conflicts later, after merging few more PRs pending

Copy link
Contributor

@acarbonetto acarbonetto left a 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?

@Yury-Fridlyand
Copy link
Collaborator Author

No

* @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.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

* @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();
Copy link
Collaborator

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?

Copy link
Collaborator

@barshaul barshaul Apr 10, 2024

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

Copy link
Collaborator Author

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;
Copy link
Collaborator

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)

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator

@barshaul barshaul left a 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]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@acarbonetto acarbonetto merged commit 4c9c9ff into valkey-io:main Apr 11, 2024
45 checks passed
@acarbonetto acarbonetto deleted the java/integ_yuryf_SETRANGE branch April 11, 2024 19:48
alex-arzola-imp pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Apr 12, 2024
* 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]>
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants