Skip to content

Add HSTRLEN command implementation and testing #1241

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.

Sign up for GitHub

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

Jump to bottom
Merged
NickCraver merged 8 commits into from
Oct 19, 2019
Merged

Add HSTRLEN command implementation and testing #1241

NickCraver merged 8 commits into from
Oct 19, 2019

Conversation

@eitanhs
Copy link
Contributor

@eitanhs eitanhs commented Sep 27, 2019

implements #1238

@eitanhs
Copy link
Contributor Author

eitanhs commented Sep 27, 2019 •
edited
Loading

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
Closed
@mgravell
Copy link
Collaborator

mgravell 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?

@eitanhs
Copy link
Contributor Author

eitanhs commented Sep 28, 2019 •
edited
Loading

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

NickCraver commented Oct 14, 2019

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...

NickCraver
NickCraver reviewed Oct 14, 2019
View reviewed changes
@eitanhs eitanhs requested a review from NickCraver October 17, 2019 12:17
NickCraver
NickCraver requested changes Oct 19, 2019
View reviewed 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?

@eitanhs eitanhs requested a review from NickCraver October 19, 2019 14:31
@NickCraver
Copy link
Collaborator

NickCraver commented Oct 19, 2019

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

NickCraver
NickCraver approved these changes Oct 19, 2019
View reviewed 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.

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
@eitanhs @ttingen
Add HSTRLEN command implementation and testing (StackExchange#1241)
52efd6a 
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
@eitanhs @ttingen
Add HSTRLEN command implementation and testing (StackExchange#1241)
9b3679f 
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
@eitanhs @ttingen
Add HSTRLEN command implementation and testing (StackExchange#1241)
859a476 
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
ttingen pushed a commit to ttingen/StackExchange.Redis that referenced this pull request Nov 19, 2019
@eitanhs @ttingen
Add HSTRLEN command implementation and testing (StackExchange#1241)
717d7b7 
Implements StackExchange#1238 - adds `HSTRLEN` command and testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NickCraver NickCraver NickCraver approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eitanhs @mgravell @NickCraver @eharshosh