Add Time() and Nanos() methods #31

rkuris · 2018-07-18T21:03:29Z

These methods can be used to extract time values
from V1 UUIDs

coveralls · 2018-07-18T21:04:57Z

Pull Request Test Coverage Report for Build 92

15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 99.104%

Totals
Change from base Build 90:	0.04%
Covered Lines:	332
Relevant Lines:	335

💛 - Coveralls

coveralls · 2018-07-18T21:04:57Z

Pull Request Test Coverage Report for Build 134

14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 99.143%

Totals
Change from base Build 128:	0.04%
Covered Lines:	347
Relevant Lines:	350

💛 - Coveralls

rkuris · 2018-07-18T21:09:00Z

I'd gladly stop using my fork if this can be maintained by gofrs; suggest bumping to revision 2.1.0 when adding this functionality.

rkuris · 2018-07-18T21:28:01Z

@jbsturgeon @michaelrios your upvote here would be appreciated.

I strongly support moving away from satori/go.uuid, and this looks like a good set of developers to steward such an important library forward.

cameracker · 2018-07-18T23:25:41Z

uuid.go

@@ -153,6 +156,28 @@ func (u *UUID) SetVariant(v byte) {
 	}
 }

+// Nanos returns the number of nanoseconds in a V1 UUID
+func (u *UUID) Nanos() (int64, error) {


my recommendation would be to name this UnixNanoOf() to be consistent with the naming convention from the time library (time.Now().UnixNano())

Good suggestion. Completed.

cameracker · 2018-07-18T23:26:26Z

uuid.go

+
+// Time returns the time embedded in a V1 UUID
+// An error is returned if this is not a V1 UUID
+func (u *UUID) Time() (time.Time, error) {


Recommend naming TimeOf(), per @theckman's idea

theckman

@rkuris thank you for being the first outside contributor to this project. 👍

I like the direction we took with changing the method names from the original PR. I do have some small things we can tweak to make this PR even more polished, in my opinion.

theckman · 2018-07-19T00:31:27Z

uuid.go

@@ -153,6 +156,28 @@ func (u *UUID) SetVariant(v byte) {
 	}
 }

+// UnixNanoOf returns the number of nanoseconds in a V1 UUID


Just to be very clear to consumers, would you mind adding a quip about it being the number of nanoseconds since the UNIX epoch? I definitely don't want to nitpick copy, but I'm thinking something like:

// UnixNanoOf returns the number of nanoseconds elapsed since January 1, 1970 UTC in a V1 UUID

It might also be good to indicate that an error is returned for a non-V1 UUID here too (like we did with TimeOf).

Finally, please add punctuation to the sentence just so that the documentation reads better.

theckman · 2018-07-19T00:35:30Z

uuid.go

+	low := binary.BigEndian.Uint32(u[0:4])
+	mid := binary.BigEndian.Uint16(u[4:6])
+	hi := binary.BigEndian.Uint16(u[6:8]) & 0xfff
+	return 100 * (int64(low) + (int64(mid) << 32) + (int64(hi) << 48) - int64(epochStart)), nil


When you have a moment would you please add some spacing to this block of code and put the return calculation on a separate line (for readability purposes)? For spacing I'm just thinking of an empty line before the low line and after the hi line.

For the return, I think it'd be okay to put the calculation on its own line so that we can then have return var, nil. For me at least, it made it easier to grok on a quick look.

theckman · 2018-07-19T00:39:22Z

uuid.go

+
+// TimeOf returns the time embedded in a V1 UUID
+// An error is returned if this is not a V1 UUID
+func (u *UUID) TimeOf() (time.Time, error) {


Would you mind converting this to a function on the package that takes the UUID as input? Because this method only takes a UUID, I think it's more appropriate to be a function. If the UUID wasn't the input to the method, but was the state needed for the method to take action on the input, then I'd say 👍 on it being a method.

theckman · 2018-07-19T00:39:37Z

uuid.go

@@ -153,6 +156,28 @@ func (u *UUID) SetVariant(v byte) {
 	}
 }

+// UnixNanoOf returns the number of nanoseconds in a V1 UUID
+func (u *UUID) UnixNanoOf() (int64, error) {


Would you be able to make this a package function too?

theckman · 2018-07-19T00:41:51Z

uuid.go

+	return 100 * (int64(low) + (int64(mid) << 32) + (int64(hi) << 48) - int64(epochStart)), nil
+}
+
+// TimeOf returns the time embedded in a V1 UUID


This may seem pedantic, but would you be able to add punctuation after UUID on this line? I'll make the GoDoc read better.

theckman · 2018-07-19T00:43:23Z

generator_test.go

+	t.Run("TestTimeOfErrors", testTimeOfErrors)
+}
+
+func testTimeOfErrors(t *testing.T) {


Can we also add a test that would use a constant UUID, and test to make sure that UnixNanosOf() and TimeOf() parse the time out of it properly?

acln0

Thanks! A few minor nitpicks from me too.

acln0 · 2018-07-19T03:48:27Z

generator_test.go

@@ -45,6 +45,7 @@ func testNewV1(t *testing.T) {
 	t.Run("FaultyRand", testNewV1FaultyRand)
 	t.Run("MissingNetwork", testNewV1MissingNetwork)
 	t.Run("MissingNetworkFaultyRand", testNewV1MissingNetworkFaultyRand)
+	t.Run("V1Ordering", testNewV1Ordering)


Please change "V1Ordering" to "Ordering", since this is a subtest of testNewV1.

acln0 · 2018-07-19T03:59:28Z

generator_test.go

+		t.Fatal(err)
+	}
+	if n1 > n2 {
+		t.Errorf("generated older UUID: %s > %s", u1, u2)


Perhaps this message should print the nanoseconds, like the one below prints time.

acln0 · 2018-07-19T04:00:41Z

generator_test.go

+	t.Run("TestTimeOfErrors", testTimeOfErrors)
+}
+
+func testTimeOfErrors(t *testing.T) {


This test does not test a property of NewV2, so I don't think it should be a subtest of testNewV2.

zerkms · 2018-07-19T07:35:41Z

After giving it some more thoughts and reading the spec I came with the following new types to introduce

// 100s of nanos since 00:00:00.00, 15 October 1582
type V1Timestamp int64

func TimestampFromV1(UUID) (V1Timestamp, error)

func TimeFromV1Timestamp(V1Timestamp) (time.Time, error)

Rationale behind it:

Those are functions not methods of UUID since they make sense only for V1 and not the other types of UUID
The V1Timestamp type specifically chosen according the RFC terminology (https://tools.ietf.org/html/rfc4122#section-4.1.4)
V1Timestamp values are comparable by themselves, if one needs exact time - there is a converting function for convenience

acln0 · 2018-07-19T07:48:11Z

@zerkms I like this API very much. It's a little verbose, but it's very explicit, and it encodes the notion that timestamps are 100s of nanoseconds since the Gregorian calendar in a type. Big +1 from me.

niaow · 2018-07-19T12:30:20Z

Would it be a better idea to do V1Timestamp.Time() or TimeFromV1Timestamp(V1Timestamp)?

acln0 · 2018-07-19T14:33:45Z

I have no opinion on the method vs. top-level function matter, but I do like the proposed data type, and I think we should use it.

rkuris · 2018-07-19T19:31:38Z

+1 from me on the new type. I will work on the proposed changes. Since there is a lot of indifference in the method vs function debate, how about we just offer both?

niaow · 2018-07-19T20:33:58Z

@rkuris it doesn't really make sense to do both. I would be fine with either.

theckman · 2018-07-21T04:49:27Z

I would prefer we only offer a function, and not both.

acln0 · 2018-07-21T05:08:10Z

I agree with not offering both. Let's have one, obvious way to do things.

That being said, an argument can be made for the method variant, based on prior cases. Note, for example, that the time package offers https://golang.org/pkg/time/#Time.Minute and https://golang.org/pkg/time/#Time.Month rather than MinuteFromTime and MonthFromTime.

I wouldn't be opposed to an API like

func (u UUID) V1Timestamp() (V1Timestamp, error)

and

func (t V1Timestamp) Time() (time.Time, error)

Both this and the function-based API read well to me. I suppose it's a question of taste and style. We need a resolution nevertheless, so we can move forward with the proposal.

theckman · 2018-07-21T05:10:45Z

I think I'm hesitant to have it on the type because not every UUID is a V1 UUID. I'm looking at it from the perspective of what the implicit meaning of the implementation is. I think having it on the type implies the wrong thing.

acln0 · 2018-07-21T05:16:42Z

@theckman Thanks. That's a very good point, and I am now leaning strongly towards the function variant as well.

theckman · 2018-07-28T04:11:21Z

We've had quite a few discussions in this PR, so I think it'd be good to summarize what we've talked about.

I think the agreement is that we want to create a new unsigned integer timestamp type [1], to represent the number of 100-nanosecond intervals since 00:00:00.00, 15 October 1582 [2]. This is to be closer to the RFC and how it thinks about time. A user will obtain one of these timestamps by calling a function on the package, not a method on a type within the package, while giving it a V1 UUID which it can parse the time out of.

To then get a time.Time value, I'm of the opinion that it should be a method on this timestamp type (taking inspiration from the time package).

I like the idea of the proposal so far, but I've been thinking and I want to propose a slight tweak and see what people think. Based on reading the RFC again, the "timestamp" of V2 ~ V5 UUIDs isn't actually a timestamp at all, confirming that only applies to V1. So with this I think specifying V1 is a bit redundant. We can just mention it in the docs for those who are familiar with the RFC.

Here's what I was thinking of, to expand on the example above:

// Timestamp is the count of 100-nanosecond intervals since 00:00:00.00,
// 15 October 1582 within a V1 UUID. This type has no meaning for
// V2 ~ V5 UUIDs as they don't have an embedded timestamp .
type Timestamp uint64

// Time returns the UTC time.Time representation of the Timestamp.
func (t Timestamp) Time() (time.Time, error) {}

// TimestampFromV1 returns the Timestamp embedded within a V1 UUID.
// Returns an error if the UUID is any version other than 1.
func TimestampFromV1(u UUID) (Timestamp, error) {}

What does everyone think? Would this be acceptable for everyone as the API we want to support?

[1] https://tools.ietf.org/html/rfc4122#section-4.2.2
[2] https://tools.ietf.org/html/rfc4122#section-4.1.4

theckman · 2018-07-29T23:39:21Z

@rkuris let us know if there's anything we can do to help with this PR.

rkuris · 2018-07-30T00:42:15Z

Should be able to code this up this week. Thanks for all the great input!

…

On Sun, Jul 29, 2018, 4:39 PM Tim Heckman ***@***.***> wrote: @rkuris <https://github.com/rkuris> let us know if there's anything we can do to help with this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADC47AWMMJwL2GyZdwAqUN4inQJelY6nks5uLkeqgaJpZM4VVXBi> .

theckman · 2018-08-11T20:54:03Z

@rkuris I wanted to follow back up and see if there's anything we can do to help you with this PR. I think we'd be happy to adapt your work and raise a separate PR, if you're okay with that. If we do go that route, I'd like to do it in a way that tries to make sure we are able to still attribute the contribution to you in some capacity.

rkuris · 2018-08-13T15:15:27Z

Sorry -- I had some work issues pop up. If I can't complete it this week, then by all means, take over and implement it. I don't think it's a lot of work. Ron

…

On Sat, Aug 11, 2018 at 1:54 PM Tim Heckman ***@***.***> wrote: @rkuris <https://github.com/rkuris> I wanted to follow back up and see if there's anything we can do to help you with this PR. I think we'd be happy to adapt your work and raise a separate PR, if you're okay with that. If we do go that route, I'd like to do it in a way that tries to make sure we are able to still attribute the contribution to you in some capacity. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADC47EZeghLE8xF5dRuZyT7mj2wJQBhEks5uP0RsgaJpZM4VVXBi> .

rkuris · 2018-08-18T13:42:24Z

Testing the edge cases turned up a bug in how times were constructed in the previous PR. I'll make a note in the satori.uuid repo to that effect.

rkuris · 2018-08-21T21:26:27Z

Greetings,

I think all the comments have been addressed by this rewrite. Is this ready to merge now or does this need more work?

zerkms · 2018-08-21T21:32:43Z

uuid.go

+const _100nsPerSecond = 10000000
+
+// Time returns the UTC time.Time representation of a Timestamp
+func (t Timestamp) Time() (time.Time, error) {


Is error as a second return value here for potential unknown cases in future to avoid BCs, or is there any other reason?

I'm not against, or pro- having it, just curious.

I think that's the idea. You can use Must(thing.Time()) if you don't care about errors.

zerkms · 2018-08-21T21:33:50Z

uuid.go

+	low := binary.BigEndian.Uint32(u[0:4])
+	mid := binary.BigEndian.Uint16(u[4:6])
+	hi := binary.BigEndian.Uint16(u[6:8]) & 0xfff
+	return Timestamp(low) + (Timestamp(mid) << 32) + (Timestamp(hi) << 48), nil


I think using Timestamp type here is confusing.

How about doing math + bit shift operations with uint64 first?

A Timestamp is a uint64, and we need to return one. Why is it confusing?

I'm a proponent of coding against types: I strongly believe that types carry a lot of extra context.

In this case, keeping in mind that Timestamp is a type that represents number of 100s of nanoseconds since 00:00:00.00 15 October 1582, the Timestamp(mid) << 32 operation makes very little sense: what does "shift 100s of nanoseconds 32 bits left" mean?

It's a mathematical operation and should be applied to numbers, which then (in the very end) might be interpreted as something domain-specific.

PS: it's my personal opinion, if other people are okay - I won't insist on this change too hard.

I changed it. I don't have a strong opinion either way, but to me it seems harder to read after making the change (a ton of casting is now happening).

I suppose I could break it up into multiple lines but it still seems "wordier". I don't have a strong opinion though.

It looks ideal to me now :-)

acln0 · 2018-08-21T21:40:42Z

uuid_test.go

@@ -133,3 +134,51 @@ func TestMust(t *testing.T) {
 	}
 	Must(fn())
 }
+
+func TestTimeFromTimestamp(t *testing.T) {
+	tests := []struct {


gofmt please.

Following the code review suggestions, added a new Timestamp type, and broke the code into two functions, one to get the timestamp from a V1 UUID and one to convert the timestamp to a time.Time value.

Also changed from Timestamp shifts to uint64 shifts

acln0 · 2018-08-21T21:51:59Z

LGTM now. Ping @theckman.

theckman · 2018-08-22T07:31:25Z

I'll review / merge tomorrow morning (in ~8 hours).

theckman

Thank you so much for this contribution!

Add Time() and Nanos() methods Signed-off-by: Tim Heckman <t@heckman.io>

theckman · 2018-08-22T15:40:01Z

New release cut that includes this change: https://github.com/gofrs/uuid/releases/tag/v3.1.0

cameracker reviewed Jul 18, 2018

View reviewed changes

rkuris force-pushed the master branch 2 times, most recently from c8e54eb to f772d06 Compare July 19, 2018 00:27

theckman requested changes Jul 19, 2018

View reviewed changes

acln0 requested changes Jul 19, 2018

View reviewed changes

theckman added this to the 2.2.0 milestone Jul 23, 2018

theckman mentioned this pull request Jul 23, 2018

Outstanding open issues / PRs in satori/go.uuid #32

Closed

theckman modified the milestones: 2.2.0, 2.3.0 Jul 28, 2018

niaow added the enhancement New feature or request label Jul 28, 2018

rkuris force-pushed the master branch from f772d06 to 9d4d2aa Compare August 18, 2018 06:16

rkuris mentioned this pull request Aug 18, 2018

Add Time() and Nanos() methods satori/go.uuid#50

Closed

zerkms reviewed Aug 21, 2018

View reviewed changes

acln0 requested changes Aug 21, 2018

View reviewed changes

Reimplementation of timestamps from V1 UUIDs

b69f8a4

Following the code review suggestions, added a new Timestamp type, and broke the code into two functions, one to get the timestamp from a V1 UUID and one to convert the timestamp to a time.Time value.

rkuris force-pushed the master branch from 9d4d2aa to b69f8a4 Compare August 21, 2018 21:46

gofmt changes

bfaa575

Also changed from Timestamp shifts to uint64 shifts

acln0 approved these changes Aug 21, 2018

View reviewed changes

zerkms approved these changes Aug 21, 2018

View reviewed changes

theckman approved these changes Aug 22, 2018

View reviewed changes

theckman merged commit bfaa575 into gofrs:master Aug 22, 2018

theckman added a commit that referenced this pull request Aug 22, 2018

Merge pull request #31 from rkuris/master

d41eeda

Add Time() and Nanos() methods Signed-off-by: Tim Heckman <t@heckman.io>

Add Time() and Nanos() methods #31

Add Time() and Nanos() methods #31

Conversation

rkuris commented Jul 18, 2018

coveralls commented Jul 18, 2018

Pull Request Test Coverage Report for Build 92

💛 - Coveralls

coveralls commented Jul 18, 2018 • edited

Pull Request Test Coverage Report for Build 134

💛 - Coveralls

rkuris commented Jul 18, 2018

rkuris commented Jul 18, 2018

cameracker Jul 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theckman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acln0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zerkms commented Jul 19, 2018

acln0 commented Jul 19, 2018 • edited

niaow commented Jul 19, 2018 • edited

acln0 commented Jul 19, 2018

rkuris commented Jul 19, 2018 via email

niaow commented Jul 19, 2018

theckman commented Jul 21, 2018

acln0 commented Jul 21, 2018

theckman commented Jul 21, 2018 • edited

acln0 commented Jul 21, 2018

theckman commented Jul 28, 2018 • edited

theckman commented Jul 29, 2018

rkuris commented Jul 30, 2018 via email

theckman commented Aug 11, 2018

rkuris commented Aug 13, 2018 via email

rkuris commented Aug 18, 2018

rkuris commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acln0 commented Aug 21, 2018 • edited

theckman commented Aug 22, 2018

theckman left a comment

Choose a reason for hiding this comment

theckman commented Aug 22, 2018

coveralls commented Jul 18, 2018 •

edited

cameracker Jul 18, 2018 •

edited

acln0 commented Jul 19, 2018 •

edited

niaow commented Jul 19, 2018 •

edited

theckman commented Jul 21, 2018 •

edited

theckman commented Jul 28, 2018 •

edited

acln0 commented Aug 21, 2018 •

edited