UUID continuation by jkthorne · Pull Request #4453 · crystal-lang/crystal

jkthorne · 2017-05-23T21:23:43Z

I wanted to continue the work of UUID on crystal from #2716

ysbaddaden

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

ysbaddaden · 2017-05-23T21:50:48Z

spec/std/uuid_spec.cr

+
+  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"


We have Slice#hexstring maybe we could have a symetric UUID#hexstring?

ysbaddaden · 2017-05-23T21:50:51Z

spec/std/uuid_spec.cr

+
+  it "compares to strings" do
+    uuid = UUID.new "c3b46146eb794e18877b4d46a10d1517"
+    ->{ uuid == "c3b46146eb794e18877b4d46a10d1517" }.call.should eq(true)


Wouldn't uuid.should eq("c3b46146eb794e18877b4d46a10d1517") work?

ysbaddaden · 2017-05-23T21:57:52Z

src/uuid.cr

+# versions.
+struct UUID
+  # Internal representation.
+  @bytes = StaticArray(UInt8, 16).new(UInt8.new(0))


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

ysbaddaden · 2017-05-23T22:01:08Z

src/uuid/rfc4122.cr

+    UUID.byte_version @bytes[6]
+  end
+
+  # Sets variant to a specified `value`. Doesn't set variant (see `UUID#variant=(value : Variant)`).


Typo: "Sets version".

ysbaddaden · 2017-05-23T22:13:07Z

src/uuid/empty.cr

@@ -0,0 +1,14 @@
+struct UUID
+  # Empty UUID.
+  @@empty_bytes = StaticArray(UInt8, 16).new { 0_u8 }


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.

ysbaddaden · 2017-05-23T22:15:12Z

src/uuid/rfc4122.cr

+  def initialize(version : Version)
+    case version
+    when Version::V4
+      @bytes.to_unsafe.copy_from SecureRandom.random_bytes(16).pointer(16), 16


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

ysbaddaden · 2017-05-23T22:16:14Z

src/uuid/rfc4122.cr

+
+  {% for v in %w(1 2 3 4 5) %}
+
+    # Returns `true` if UUID looks like V{{ v.id }}, `false` otherwise.


Maybe "If UUID is a Vx".

ysbaddaden · 2017-05-23T22:20:46Z

src/uuid/variant.cr

+  end
+
+  # Returns UUID variant based on provided 8th `byte` (0-indexed).
+  def self.byte_variant(byte : UInt8)


We don't need the byte_variant class methods. They should be inlined into #variant and #variant=.

I dont think I know what you mean here

ysbaddaden · 2017-05-23T22:23:17Z

src/uuid/rfc4122.cr

+    end
+  end
+
+  # Returns version based on provided 6th `byte` (0-indexed).


We don't need the byte_version class methods. They should be inlined in #version and #version=.

I still don't think I get what you mean by this

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.

Sija · 2017-05-24T20:51:45Z

src/uuid.cr

+    @bytes.to_unsafe
+  end
+
+  # Writes hyphenated format string to `io`.


`io` -> the *io*

Sija · 2017-05-24T20:52:51Z

src/uuid/rfc4122.cr

+    UUID.byte_version @bytes[6]
+  end
+
+  # Sets Version to a specified `value`. Doesn't set variant (see `UUID#variant=(value : Variant)`).


Version -> `version`

Sija · 2017-05-24T20:53:50Z

src/uuid/rfc4122.cr

+
+  {% for v in %w(1 2 3 4 5) %}
+
+    # Returns `true` if UUID looks is a Vx, `false` otherwise.


Vx -> `V{{ v.id }}`

yes, I didn't mean to drop the macro, it was great!

Sija · 2017-05-24T20:54:05Z

src/uuid/rfc4122.cr

+      variant == Variant::RFC4122 && version == RFC4122::Version::V{{ v.id }}
+    end
+
+    # Returns `true` if UUID looks is a Vx, raises `Error` otherwise.


Sija · 2017-05-24T20:54:32Z

src/uuid/string.cr

+    end
+  end
+
+  #  Returns string in specified `format`.


`format` -> *format*

Sija · 2017-05-24T20:55:02Z

src/uuid/variant.cr

+    UUID.byte_variant @bytes[8]
+  end
+
+  # Sets UUID variant to specified `value`.


`value` -> *value*

ysbaddaden

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?

jkthorne · 2017-11-08T01:00:37Z

src/uuid.cr

  end

-  # Returns `true` if `other` string represents the same UUID, `false` otherwise.
-  def ==(other : String)


Why did you remove this?

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.

Sija · 2017-11-08T01:02:18Z

src/uuid.cr

    new(bytes, variant, version)
  end

+  def self.new(uuid : UUID, variant = nil, version = nil)


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?

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

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.

~~If it's a copy constructor shouldn't the variant and version be taken from the given uuid?~~

RX14 · 2017-11-08T01:08:03Z

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.

jkthorne · 2017-11-08T01:09:59Z

Yeah I would like to see this in a PR personally there is just a lot of changes here.
But I would like to separate it out.

RX14 · 2017-11-08T23:16:48Z

So you want me to create a PR instead of yours?

jkthorne · 2017-11-09T05:29:40Z

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

RX14 · 2017-11-09T13:37:45Z

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

RX14 · 2017-11-20T12:21:24Z

This pr seems to be stalled. I suggest we review it and attempt to merge it as-is.

jkthorne · 2017-11-20T19:24:56Z

Another option is I can just release this as a library and remove it from the std lib

RX14 · 2017-11-20T20:44:00Z

I believe it belongs in the stdlib.

jkthorne · 2017-11-21T00:01:07Z

Is there a path to get this merged?

RX14 · 2017-11-21T14:54:49Z

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

mverzilli · 2017-11-21T14:57:34Z

@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!

RX14 · 2017-11-21T15:39:04Z

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

mverzilli · 2017-11-23T20:54:20Z

src/uuid.cr

+  end
+
+  {% for v in %w(1 2 3 4 5) %}
+    # Returns `true` if UUID looks is a V{{ v.id }}, `false` otherwise.


Did you mean "if UUID looks as"?

"If UUID has version Vx" would make more sense surely?

mverzilli · 2017-11-23T20:54:31Z

src/uuid.cr

+      variant == Variant::RFC4122 && version == RFC4122::Version::V{{ v.id }}
+    end
+
+    # Returns `true` if UUID looks is a V{{ v.id }}, raises `Error` otherwise.


mverzilli · 2017-11-23T21:25:20Z

I think it looks good overall, only one question:

Why are we constantly using to_s to compare UUIDs in specs? Shouldn't we directly compare UUID instances?

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.

RX14 · 2017-11-23T21:32:40Z

@mverzilli as long as the to_s specs properly test to_s, I don't see the problem. And as you said, this way is much neater.

Old

mverzilli · 2017-11-24T12:43:11Z

Thank you @wontruefree for the great work!

mirek · 2018-02-04T12:00:16Z

Thank you for finishing this.

jkthorne · 2018-02-10T17:30:35Z

@mirek you are welcome. Thank you for getting it started.

mirek and others added 8 commits June 3, 2016 18:15

Add UUID.

3b1bae7

Rearrange structure to separate RFC4122 logic.

f0bb4d8

Updates.

a9019b6

Merge branch 'master' into uuid

cbfd749

Merge branch 'master' into uuid

a2e23a3

Merge remote-tracking branch 'origin/master' into uuid

3d69d69

remove uuid from prelude; update for moden build for crystal

133e84d

Merge remote-tracking branch 'origin/master' into uuid

c6a9604

ysbaddaden requested changes May 23, 2017

View reviewed changes

jkthorne added 3 commits May 24, 2017 13:52

Use should and eq instead of checking against true

0ca59d6

fix typo

2d881d9

Doc fix

e48a7d5

Sija reviewed May 24, 2017

View reviewed changes

src/uuid.cr Outdated

@bytes.to_unsafe

end

# Writes hyphenated format string to `io`.

Copy link

Contributor

Sija May 24, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

`io` -> the *io*

Sija reviewed May 24, 2017

View reviewed changes

src/uuid/string.cr Outdated

end

end

# Returns string in specified `format`.

Copy link

Contributor

Sija May 24, 2017

Choose a reason for hiding this comment

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

`format` -> *format*

Sija reviewed May 24, 2017

View reviewed changes

src/uuid/variant.cr Outdated

UUID.byte_variant @bytes[8]

end

# Sets UUID variant to specified `value`.

Copy link

Contributor

Sija May 24, 2017

Choose a reason for hiding this comment

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

`value` -> *value*

jkthorne and others added 3 commits May 24, 2017 23:00

Change the implimintation of UUID::EMPTY

a48e4fb

Update Docs

64e64f7

Merge branch 'master' into uuid

df3fd11

mverzilli added kind:feature status:accepted topic:stdlib labels May 26, 2017

Jack Thorne added 4 commits May 26, 2017 11:38

Change formatting form to_s to methods

55ad17a

Merge branch 'uuid' of github.com:wontruefree/crystal into uuid

c1cf14d

wip

8295cfa

wip

b7c17f3

ysbaddaden requested changes May 31, 2017

View reviewed changes

remove trasition to Random

c6b781c

jkthorne commented Nov 8, 2017

View reviewed changes

Sija reviewed Nov 8, 2017

View reviewed changes

Clarify doc for UUID.new(UUID, ...)

9c49e7b

RX14 assigned ysbaddaden and unassigned ysbaddaden Nov 12, 2017

RX14 approved these changes Nov 21, 2017

View reviewed changes

mverzilli reviewed Nov 23, 2017

View reviewed changes

mverzilli approved these changes Nov 24, 2017

View reviewed changes

mverzilli merged commit 1b669c1 into crystal-lang:master Nov 24, 2017

mverzilli added this to the Next milestone Nov 24, 2017

jkthorne deleted the uuid branch February 10, 2018 17:30


		{% for v in %w(1 2 3 4 5) %}

		# Returns `true` if UUID looks like V{{ v.id }}, `false` otherwise.


		{% for v in %w(1 2 3 4 5) %}

		# Returns `true` if UUID looks is a Vx, `false` otherwise.

Uh oh!

Conversation

jkthorne commented May 23, 2017

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sija May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sija Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 commented Nov 8, 2017

Uh oh!

jkthorne commented Nov 8, 2017

Uh oh!

RX14 commented Nov 8, 2017

Uh oh!

jkthorne commented Nov 9, 2017

Uh oh!

RX14 commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 commented Nov 20, 2017

ysbaddaden May 23, 2017 •

edited

Loading

Sija May 24, 2017 •

edited

Loading

RX14 Nov 8, 2017 •

edited

Loading

Sija Nov 8, 2017 •

edited

Loading

RX14 commented Nov 9, 2017 •

edited

Loading