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

API Changes #2

Closed
pborman opened this issue Feb 19, 2016 · 10 comments
Closed

API Changes #2

pborman opened this issue Feb 19, 2016 · 10 comments

Comments

@pborman
Copy link
Collaborator

pborman commented Feb 19, 2016

Please comment in this issue about any requested API changes.

@bmatsuo
Copy link
Contributor

bmatsuo commented Feb 24, 2016

I think the text/template style of Must would make more sense in this package.. I made this comment in pborman/uuid#3 (comment) but I don't think it was ever addressed or responded to. It's fine if you disagree but I want to make sure you did consider it. Here's the excerpt my original comment

  • Consider a function func Must(uuid UUID, err error) UUID so that you don't need Must* Variants for each type of UUID.
u := uuid.Must(uuid.NewUUID())
// ...
  • I think you could probably have a NewRandom function that returns an error. Then you can document that New() is an alias for uuid.Must(uuid.NewRandom()). I don't really know under what conditions rand.Reader.Read() returns an error but I am a fan of symmetry.

@pborman
Copy link
Collaborator Author

pborman commented Feb 24, 2016

Thank you for bringing that up again. Yes, it sort of go lost in the mix. I think it makes a lot of sense. Would you like to create a pull request to do this? The existing MustParse can just go away then. I would be happy to do it, but it would be nice for the history to show it came from you.

@bmatsuo
Copy link
Contributor

bmatsuo commented Feb 24, 2016

Sure thing, #3

@bmatsuo
Copy link
Contributor

bmatsuo commented Feb 24, 2016

During those changes I noticed that the global variables don't seem to conform to Go best practices (e.g. code review wiki). In addition to use of underscores and CAPS (i.e. NIL) I think the capitalization of NameSpace is incorrect because namespace is a single word.

It seems like the global variables should have the following names

var (
    NamespaceDNS  = ...
    NamespaceURL  = ...
    NamespaceOID  = ...
    NamespaceX500 = ...
    Nil           UUID
)

@bmatsuo
Copy link
Contributor

bmatsuo commented Feb 28, 2016

@pborman any thoughts on the proposed name changes above? I've took a real pass through the API and came up with a couple other suggestions.

  • Instead of NIL/Nil, which is intentionally a misnomer, perhaps Zero or Z could be used instead. Personally I think the variable could simply be removed completely, but I don't think it's really a big deal..
  • The method UUID.Id() should be UUID.ID(). In the context of NodeID capital letters are used consistently. The Id() method seems to be the only inconsistent usage.

@pborman
Copy link
Collaborator Author

pborman commented Feb 29, 2016

The name Nil is correct. From RFC 4122:

4.1.7. Nil UUID
The nil UUID is special form of UUID that is specified to have all 128 bits set to zero.

The other name changes look reasonable.

@pborman
Copy link
Collaborator Author

pborman commented Feb 29, 2016

I have push fixes for the naming issues, including a few others with non-exported names.

@bmatsuo
Copy link
Contributor

bmatsuo commented Feb 29, 2016

Indeed that is my mistake about the Nil UUID. I didn't realize that name was part of the specification.

@meyerzinn
Copy link

Could there be a function to compare the equality of UUIDs?

@pborman
Copy link
Collaborator Author

pborman commented May 22, 2017

There is no need for a function, arrays (which is what the UUID type is) are directly comparable.

@pborman pborman closed this as completed Aug 28, 2018
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

No branches or pull requests

3 participants