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 LRange and LTrim commands. (List Commands) #1041

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

SanHalacogluImproving
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SanHalacogluImproving SanHalacogluImproving requested a review from a team as a code owner February 26, 2024 22:52
@SanHalacogluImproving SanHalacogluImproving changed the title Java: Add LRange command. (List Commands) Java: Add LRange and LTrim commands. (List Commands) Feb 26, 2024
@acarbonetto acarbonetto added the java issues and fixes related to the java client label Feb 27, 2024
* @param elements The elements to insert at the tail of the list stored at <code>key</code>.
* @return Command Response - The length of the list after the push operations.
*/
public T rpush(@NonNull String key, @NonNull String[] elements) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just diff issue? Or did you move these?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move new functions to the end to avoid this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am roughly following the order that they had in the Node client and Py. (they differ from each other a bit). This was just a rebase mistake at some point so i just moved it to the correct spot.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since git fails to detect that you added few functions in the middle, you have to reorder them. The actual order does not matter, I added all commends to the end to simplify reviewing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved
* @param elements The elements to insert at the tail of the list stored at <code>key</code>.
* @return Command Response - The length of the list after the push operations.
*/
public T rpush(@NonNull String key, @NonNull String[] elements) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move new functions to the end to avoid this

java/client/src/test/java/glide/api/RedisClientTest.java Outdated Show resolved Hide resolved

/**
* Trims an existing list so that it will contain only the specified range of elements specified.
* <br>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here
is in a new line, but in lrange it's not, let's be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is caused by Spotless hence why it looks like that.

@@ -572,6 +523,109 @@ public T lpopCount(@NonNull String key, long count) {
return getThis();
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments

* @param elements The elements to insert at the tail of the list stored at <code>key</code>.
* @return Command Response - The length of the list after the push operations.
*/
public T rpush(@NonNull String key, @NonNull String[] elements) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@SanHalacogluImproving SanHalacogluImproving force-pushed the java/integ_SanH_add_Lrange branch from e62eac7 to 88b44ee Compare March 4, 2024 18:53
*
* @see <a href="https://redis.io/commands/rpop/">redis.io</a> for details.
* @param count The count of the elements to pop from the list.
* @returns Command Response - An array of popped elements will be returned depending on the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @returns Command Response - An array of popped elements will be returned depending on the
* @return Command Response - An array of popped elements will be returned depending on the

* @param key The key of the list.
* @param start The starting point of the range.
* @param end The end of the range.
* @return Always <code>OK</code>. <br>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return Always <code>OK</code>. <br>
* @return Always <code>OK</code>.<br>

return getThis();
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reorder to simplify the diff

@Yury-Fridlyand Yury-Fridlyand merged commit 5f687a3 into valkey-io:main Mar 4, 2024
11 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the java/integ_SanH_add_Lrange branch March 4, 2024 19:31
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
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.

4 participants