Add support for HSCAN NOVALUES by atakavci · Pull Request #2722 · StackExchange/StackExchange.Redis (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Closes/Fixes #2721
brings new functions to the API ;
- IDatabase.HashScanNoValues
- IDatabase.HashScanNoValues
- IDatabaseAsync.HashScanNoValuesAsync
to enable the return type consisting of keys in the hash.
added some unit and integration tests in paralled to what is there for HashScan and HashScanAsnyc.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes requested here but looking awesome - with tidy we should be good, thanks for this!
This comment was marked as outdated.
- add a new message.create
- test version guards
hey @WeihanLi , checking where the similar approach is applied and will try that. thanks !
where is this quote from?
This feature will be released in Redis 8.0 and is available since 7.2.5 in Valkey from my research
my 2 cents on this,,
making developers life easier and keeping client compatible in more situations/environments is cool.
on the other hand handling such scenarios and doing more (than actually needed/requested) under the hood, this introduces the possibility of significant increase in resource usage both on client and server side while people probably wont even get noticed about it (probably until something bad).
what do you think about it ?
hey @NickCraver, how about taking another look to this one?? Let me know if you have concerns or there are things to fix and i ll deal with it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; the unrelated whitespace changes are ... not ideal in a feature PR, but I think we should just eat them - they're fine
TODO (not necessarily as part of this PR, for timing): release notes
I noticed in updating this that we have a mismatch here on HashScanNoValues (2 overloads)vsHashScanNoValuesAsync` (1 method).
I'd ask when adding here: that's the purpose of the smaller overload for HashScanNoValues? It seems like we should just remove it and normalize on the one method, but maybe I'm missing a use case.
I'd ask when adding here: that's the purpose of the smaller overload for
HashScanNoValues? It seems like we should just remove it and normalize on the one method, but maybe I'm missing a use case.
hey @NickCraver,, trying to remember my reasoning.. and after a quick look back to the code, you are most probably right.
I guess it was just the sake of keeping the convention and provide same method signatures with HashScan and HashScanAsync already there.
i dont have a strong opinion on it. What do you prefer ??
@atakavci Let's trim down and remove please - usually the #1 reason for overload is backwards compat, which isn't in play here :)
@atakavci Let's trim down and remove please - usually the #1 reason for overload is backwards compat, which isn't in play here :)
done 👍
add support for hscan novalues option Add support for HSCAN NOVALUES