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 func Uint64Map Uint64s #453

Merged
merged 8 commits into from
Apr 16, 2020
Merged

add func Uint64Map Uint64s #453

merged 8 commits into from
Apr 16, 2020

Conversation

xuanskyer
Copy link
Contributor

add func Uint64Map Uint64s

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking a this, just a few little bits of feedback.

Co-Authored-By: Steven Hartland <[email protected]>
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry missed the fact there was no doc comments for the new methods in my previous review.

@stevenh
Copy link
Collaborator

stevenh commented Apr 10, 2020

Hi @xuanskyer could you address the comments above and rebase to fix the conflicts please?

@xuanskyer
Copy link
Contributor Author

Hi @xuanskyer could you address the comments above and rebase to fix the conflicts please?

done

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @xuanskyer few more things with the update and one I missed when I initially reviewed, sorry about that.

Copy link
Contributor Author

@xuanskyer xuanskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change!

@xuanskyer xuanskyer requested a review from stevenh April 15, 2020 06:20
@stevenh
Copy link
Collaborator

stevenh commented Apr 15, 2020

I think you forgot to push the update there @xuanskyer

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of fixes to the doc strings, to make them consistent with the existing methods but also missing a test for Uint64Map and validation that Uint64s works with math.MaxUint64.

redis/reply.go Outdated
@@ -479,6 +479,55 @@ func Positions(result interface{}, err error) ([]*[2]float64, error) {
return positions, nil
}

// Uint64s is a helper that converts an array command reply to a []uint64.
//If err is not equal to nil, then Uint64s returns nil, err. Nil array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//If err is not equal to nil, then Uint64s returns nil, err. Nil array
// If err is not equal to nil, then Uint64s returns nil, err. Nil array

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xuanskyer almost there, you just missed the doc comment fix, so I've added it again so you can just click 'Commit suggestion'.

Co-Authored-By: Steven Hartland <[email protected]>
@stevenh stevenh merged commit 617d952 into gomodule:master Apr 16, 2020
pull bot pushed a commit to scope-demo/redigo that referenced this pull request Apr 16, 2020
Add two additional help functions that handle uint64 data types:
* Uint64Map
* Uint64s
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.

2 participants