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

Add HSTRLEN command implementation and testing #1241

Merged

Conversation

eitanhs
Copy link
Contributor

@eitanhs eitanhs commented Sep 27, 2019

implements #1238

@eitanhs
Copy link
Contributor Author

eitanhs commented Sep 27, 2019

What version of Redis server exists in the CI environment?
The HSTRLEN command is available since 3.2.0. and the tests are failing due to 'unknown command'...

HSTRLEN command is available since 3.2.0 so added tests are passing only on Ubuntu image

@eitanhs eitanhs mentioned this pull request Sep 27, 2019
@mgravell
Copy link
Collaborator

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?

@eitanhs
Copy link
Contributor Author

eitanhs commented Sep 28, 2019

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!

@NickCraver
Copy link
Collaborator

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: Skip.IfMissingFeature(conn, nameof(RedisFeatures.Streams), r => r.Streams); to see how this should be added to features based on version and tests skipped at lower versions :)

@eitanhs
Copy link
Contributor Author

eitanhs commented Oct 14, 2019

@NickCraver - thanks for the feedback! fixed the relevant test.
Unfortunately the CI is still unhappy, but the failing tests seem unrelated to my changes...

Copy link
Collaborator

@NickCraver NickCraver left a 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!

@eitanhs
Copy link
Contributor Author

eitanhs commented Oct 19, 2019

I hope I read you correctly and you meant that I'll add another UT for the async version of the method in Strings.cs ? If so - done :)

What's weird is that there isn't any UTs for the sync version of any other method beside HashStringLength as far as I can see... is that on purpose or am I missing something?

@NickCraver
Copy link
Collaborator

You're all good - agree there's some gaps there I'll go back and normalize :)

Copy link
Collaborator

@NickCraver NickCraver left a 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

@NickCraver NickCraver changed the title added HSTRLEN command implementation and basic UT Add HSTRLEN command implementation and testing Oct 19, 2019
@NickCraver NickCraver merged commit b0a07a7 into StackExchange:master Oct 19, 2019
@eitanhs eitanhs deleted the feature/support_hstrlen_command branch October 19, 2019 14:34
@eitanhs
Copy link
Contributor Author

eitanhs commented Oct 19, 2019

Awesome, thanks!

ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants