-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add HSTRLEN command implementation and testing #1241
Add HSTRLEN command implementation and testing #1241
Conversation
|
Looks good to me. Can I confirm that you're free and able to freely contribute the code in the same terms as the project license etc? |
Sure! |
Just getting to some of this - apologies for the delay. We need to add some checks against the version here so that things work, check out the Streams test, specifically: |
@NickCraver - thanks for the feedback! fixed the relevant test. |
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.
This is looking great - only missing the corresponding test ([Fact]
) for HashStringLengthAsync
to ensure it works. I'm happy to take adding that or merge after you do, either way :) This is much appreciated!
I hope I read you correctly and you meant that I'll add another UT for the async version of the method in What's weird is that there isn't any UTs for the sync version of any other method beside |
You're all good - agree there's some gaps there I'll go back and normalize :) |
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.
Looking awesome - thanks so much for this! The full set of everything including tests is much appreciated <3
Awesome, thanks! |
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
implements #1238