redis / lettuce

Advanced Java Redis client for thread-safe sync, async, and reactive usage. Supports Cluster, Sentinel, Pipelining, and codecs.
https://lettuce.io
MIT License
5.3k stars 949 forks source link

Support HSCAN with NOVALUES argument #2816

Closed gerzse closed 2 months ago

gerzse commented 3 months ago

Issue #2763

HSCAN has a new argument called NOVALUES. The effect is that only the keys in the hash are returned, without associated values.

Make sure that:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 88.50575% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 77.68%. Comparing base (43843bf) to head (d98c07a). Report is 248 commits behind head on main.

Files Patch % Lines
.../api/coroutines/RedisHashCoroutinesCommandsImpl.kt 0.00% 4 Missing :warning:
src/main/java/io/lettuce/core/ScanIterator.java 76.92% 2 Missing and 1 partial :warning:
src/main/kotlin/io/lettuce/core/ScanFlow.kt 70.00% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2816 +/- ## ============================================ - Coverage 78.71% 77.68% -1.03% - Complexity 6786 7225 +439 ============================================ Files 508 537 +29 Lines 22765 24462 +1697 Branches 2446 2608 +162 ============================================ + Hits 17919 19004 +1085 - Misses 3717 4250 +533 - Partials 1129 1208 +79 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gerzse commented 2 months ago

Generally, the functionality is fine. There are some aspects that require further refinement:

  • Our lettercasing uses camel-base based on word boundaries. Since novalues is a word on its own, method names should be hscanNovalues instead of hscanNoValues. We already have some mixture in letter casing. To align with how it should be, please use a lowercase v as in Novalues.
  • Missing @since tags. The next release aims towards 7.0, so please add @since 7.0 to all newly introduced public methods. Adding the tag into the template is typically fine.
  • There are several unrelated changes such as changes to RedisServerAsyncCommands that result from committing all generated interfaces. Please remove these changes from the PR to have a focused pull request.

Thanks for the review!

gerzse commented 2 months ago

Generally, the functionality is fine. There are some aspects that require further refinement:

  • Our lettercasing uses camel-base based on word boundaries. Since novalues is a word on its own, method names should be hscanNovalues instead of hscanNoValues. We already have some mixture in letter casing. To align with how it should be, please use a lowercase v as in Novalues.
  • Missing @since tags. The next release aims towards 7.0, so please add @since 7.0 to all newly introduced public methods. Adding the tag into the template is typically fine.
  • There are several unrelated changes such as changes to RedisServerAsyncCommands that result from committing all generated interfaces. Please remove these changes from the PR to have a focused pull request.

Thanks for the review!

  • I did notice in the codebase the drift in the naming rules. I will switch to the one that you suggested. Also I'll add tags.
  • About the unrelated changes, you're right, they don't belong here. What happened is that I ran the code generator and got many differences, so I though doing something about that. I will open a separate PR only about aligning the templates with the current state of the code, if it's OK.
  • Besides this, @atakavci pointed out that I missed some interfaces that have the hscan method, so I'll try to cover those too, since hscanNovalues is a general alternative to hscan.

The code generator is a bit of Pandora's box, which I don't want to open. I ran the generator, reverted all the (quite many) unrelated changes, and kept strictly the changes related to HSCAN NOVALUES.

mp911de commented 2 months ago

@atakavci @tishun feel free to merge.