-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
UUID continuation #4453
UUID continuation #4453
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I mostly outlined a few optimizations and cleanups.
I'd like well named factories for easy usages, over having to specify a UUID::Version
, so we'd have an explicit UUID.random
instead of UUID.new(UUID::Version::V4)
.
spec/std/uuid_spec.cr
Outdated
|
||
it "supports different string formats" do | ||
UUID.new("ee843b2656d8472bb3430b94ed9077ff").to_s.should eq "ee843b26-56d8-472b-b343-0b94ed9077ff" | ||
UUID.new("3e806983-eca4-4fc5-b581-f30fb03ec9e5").to_s(UUID::Format::Hexstring).should eq "3e806983eca44fc5b581f30fb03ec9e5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have Slice#hexstring
maybe we could have a symetric UUID#hexstring
?
spec/std/uuid_spec.cr
Outdated
|
||
it "compares to strings" do | ||
uuid = UUID.new "c3b46146eb794e18877b4d46a10d1517" | ||
->{ uuid == "c3b46146eb794e18877b4d46a10d1517" }.call.should eq(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't uuid.should eq("c3b46146eb794e18877b4d46a10d1517")
work?
src/uuid.cr
Outdated
# versions. | ||
struct UUID | ||
# Internal representation. | ||
@bytes = StaticArray(UInt8, 16).new(UInt8.new(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just specify the @bytes
type:
@bytes : StaticArray(UInt8, 16)
This will require to change #initialize(String)
but could avoid a zero fill:
def initialize(value : String)
@bytes = uninitialized UInt8[16]
decode(value)
end
As well as #initialize
which can be solved with a factory:
def self.new
new(Version::V4)
end
src/uuid/rfc4122.cr
Outdated
UUID.byte_version @bytes[6] | ||
end | ||
|
||
# Sets variant to a specified `value`. Doesn't set variant (see `UUID#variant=(value : Variant)`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "Sets version".
src/uuid/empty.cr
Outdated
@@ -0,0 +1,14 @@ | |||
struct UUID | |||
# Empty UUID. | |||
@@empty_bytes = StaticArray(UInt8, 16).new { 0_u8 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a constant, along with some other namespaces:
EMPTY = UUID.new(StaticArray(UInt8, 16).new(0_u8))
DNS = UUID.new("6ba7b810-9dad-11d1-80b4-00c04fd430c8")
URL = UUID.new("6ba7b811-9dad-11d1-80b4-00c04fd430c8")
OID = UUID.new("6ba7b812-9dad-11d1-80b4-00c04fd430c8")
X500 = UUID.new("6ba7b814-9dad-11d1-80b4-00c04fd430c8")
I don't think we'd need the #empty
method anymore —since UUID is a struct it will always be copied.
src/uuid/rfc4122.cr
Outdated
def initialize(version : Version) | ||
case version | ||
when Version::V4 | ||
@bytes.to_unsafe.copy_from SecureRandom.random_bytes(16).pointer(16), 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid a copy and directly fill @bytes
, which also avoids a HEAP allocation by SecureRandom:
@bytes = uninitialized UInt8[16]
SecureRandom.random_bytes(@bytes.to_slice)
Also, maybe we could per version factories, for a nicer API?
def self.random
bytes = uninitialized UInt8[16]
SecureRandom.random_bytes(bytes.to_slice)
uuid = new(bytes)
uuid.variant = Variant::RFC4122
uuid.version = Version::V4
uuid
end
src/uuid/rfc4122.cr
Outdated
|
||
{% for v in %w(1 2 3 4 5) %} | ||
|
||
# Returns `true` if UUID looks like V{{ v.id }}, `false` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "If UUID is a Vx".
src/uuid/variant.cr
Outdated
end | ||
|
||
# Returns UUID variant based on provided 8th `byte` (0-indexed). | ||
def self.byte_variant(byte : UInt8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the byte_variant
class methods. They should be inlined into #variant
and #variant=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think I know what you mean here
src/uuid/rfc4122.cr
Outdated
end | ||
end | ||
|
||
# Returns version based on provided 6th `byte` (0-indexed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the byte_version
class methods. They should be inlined in #version
and #version=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think I get what you mean by this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply that the byte_version
class methods are only used in the version getter/setter and are thus useless. We don't need the class methods. Whatever they do should be moved to the version getter and setter.
Same for variant.
src/uuid.cr
Outdated
@bytes.to_unsafe | ||
end | ||
|
||
# Writes hyphenated format string to `io`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`io` -> the *io*
src/uuid/rfc4122.cr
Outdated
UUID.byte_version @bytes[6] | ||
end | ||
|
||
# Sets Version to a specified `value`. Doesn't set variant (see `UUID#variant=(value : Variant)`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version -> `version`
src/uuid/rfc4122.cr
Outdated
|
||
{% for v in %w(1 2 3 4 5) %} | ||
|
||
# Returns `true` if UUID looks is a Vx, `false` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vx -> `V{{ v.id }}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I didn't mean to drop the macro, it was great!
src/uuid/rfc4122.cr
Outdated
variant == Variant::RFC4122 && version == RFC4122::Version::V{{ v.id }} | ||
end | ||
|
||
# Returns `true` if UUID looks is a Vx, raises `Error` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/uuid/string.cr
Outdated
end | ||
end | ||
|
||
# Returns string in specified `format`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`format` -> *format*
src/uuid/variant.cr
Outdated
UUID.byte_variant @bytes[8] | ||
end | ||
|
||
# Sets UUID variant to specified `value`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`value` -> *value*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having the patience to apply all the suggestions! But why did you introduce Random.random_bytes
and use it when SecureRandom did a better job?
src/uuid.cr
Outdated
@@ -157,12 +157,7 @@ struct UUID | |||
@bytes.to_unsafe | |||
end | |||
|
|||
# Returns `true` if `other` string represents the same UUID, `false` otherwise. | |||
def ==(other : String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said exactly my reasoning above, it's magic, and costly, and unexpected.
Considering also the constraint that x == y
implies x.hash == y.hash
then we'd have to implement hash using to_s
too, which is costly even if you don't use this.
src/uuid.cr
Outdated
@@ -54,6 +54,10 @@ struct UUID | |||
new(bytes, variant, version) | |||
end | |||
|
|||
def self.new(uuid : UUID, variant = nil, version = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you want to pass existing UUID with different variant or version, what's the use case? Shouldn't this be a no-op?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is a copy constructor. It creates a copy of another UUID but allows you to overwrite the variant or version, it replaces variant=
and version=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I have no problem having this in general. I would not use it.
But I would not have deleted the code you did for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a copy constructor shouldn't the variant
and version
be taken from the given uuid?
I would be surprised if any core team member didn't want UUID to be immutable, but perhaps I moved too hastily and without consensus. |
Yeah I would like to see this in a PR personally there is just a lot of changes here. |
So you want me to create a PR instead of yours? |
@RX14 to be honest this is not how I am using UUID and I am not sure what the philosophy of the project is ie. immutability. but whatever gets it merged at this point I am down with. I feel like this PR is so long in the tooth I am willing to release it as a library instead of contributing upstream. |
@wontruefree well, how are you using UUID? It would be helpful to know. You can do everything in this PR that you could before I made it immutable, it's just much less confusing at the cost of some rare actions being a bit more code. |
This pr seems to be stalled. I suggest we review it and attempt to merge it as-is. |
Another option is I can just release this as a library and remove it from the std lib |
I believe it belongs in the stdlib. |
Is there a path to get this merged? |
@wontruefree yes, it just needs a positive review by another member of the core team. There might be some remaining objections but I don't see why it couldn't be merged in theory. |
@RX14 maybe it's a GitHub glitch, but what I'm seeing here is that both you and @ysbaddaden have requested changes. @wontruefree I'll review this between today and tomorrow, thanks for the hard work and patience! |
@mverzilli well I amended the PR with my own commits, so obviously I've reviewed it. As far as I know, @ysbaddaden's review is outdated. |
end | ||
|
||
{% for v in %w(1 2 3 4 5) %} | ||
# Returns `true` if UUID looks is a V{{ v.id }}, `false` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean "if UUID looks as"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If UUID has version Vx" would make more sense surely?
variant == Variant::RFC4122 && version == RFC4122::Version::V{{ v.id }} | ||
end | ||
|
||
# Returns `true` if UUID looks is a V{{ v.id }}, raises `Error` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
I think it looks good overall, only one question: Why are we constantly using I understand the code will get a bit bulkier and I'm willing to merge it as is if people don't share my opinion here, but at least I'd like to know what other folks think about this before proceeding. |
@mverzilli as long as the |
Thank you @wontruefree for the great work! |
Thank you for finishing this. |
@mirek you are welcome. Thank you for getting it started. |
I wanted to continue the work of UUID on crystal from #2716