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

Leverage github.com/google/uuid where possible #48

Merged
merged 9 commits into from
Sep 6, 2018
Merged

Leverage github.com/google/uuid where possible #48

merged 9 commits into from
Sep 6, 2018

Conversation

pborman
Copy link
Owner

@pborman pborman commented Aug 27, 2018

This change leverages the github.com/google/uuid package where reasonable. There is still duplicated code due to performance reasons.

Paul Borman added 6 commits August 27, 2018 16:10
where appropriate.  There should be no functional change with this
commit.

This has been done to reduce the amount of redundant code between
the two packages, in particular, code dealing with the clock, random
numbers, and determining the node ID from the network interfaces.

Where reasonable, this package is just a wrapper around google/uuid.
There still is duplicated code to limit the performance impact of
wrapping small functions.

The Version, Variant and Time types are now aliases of the google/uuid types.
Copy link

@harrisonhjones harrisonhjones left a comment

Choose a reason for hiding this comment

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

For whatever reason I cannot submit my other comment on marshal.go's UnmarshalText function so here it is:

The error pattern I'm used to is usually "flipped" from this:

if err != nil {
    ... handle the error ...
    ... return with wrapped error ...
}
... handle the success ...
return nil

Ie, the "happy path" is on the root indent. Any reason for the different style here? It looks like the existing code (before the change) worked this way.

.travis.yml Outdated
@@ -2,7 +2,6 @@ language: go

go:
- 1.9
- 1.10

Choose a reason for hiding this comment

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

I've seen this issue before. Fix can be found here

@pborman
Copy link
Owner Author

pborman commented Aug 28, 2018

Thanks for the review and the "travis" fix. Hopefully my latest commit will run properly.

The reason I wrote Array.UnmarshalText as:

        id, err := guuid.ParseBytes(data)
        if err == nil {
                *u = Array(id)
        }
        return err

rather than

        id, err := guuid.ParseBytes(data)
        if err != nil {
                return err
        }
        *u = Array(id)
        return nil

had two weak reasons. The first is err != nil requires two return statements meaning one more line of code. In C, years ago, the thought was there should be only 1 return path in a function so you didn't return out of the middle and miss the cleanup code. The second was to make it clear that the error from ParseBytes was in fact the error being returned (even if it was nil).

I am not tied to either version so I will modify it to do the err != nil.

@pborman
Copy link
Owner Author

pborman commented Aug 28, 2018

Resolving conflicts will be simple, but it is not clear to me if I resolve the conflict and click the "merge" button what will happen (will it merge it into the branch or merge the pull request?)

@harrisonhjones
Copy link

It looks like there are conflicts in the .travis.yml file but otherwise it looks good to merge. Also not 100% sure what happens if you merge. I would expect it to merge the PR into the master branch.

@pborman pborman merged commit adf5a74 into master Sep 6, 2018
@pborman
Copy link
Owner Author

pborman commented Sep 6, 2018

Thank you for your reviews!

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