oklog
refactor(timestamp): remove redundant `.UTC()` call and mentions
Unix timestamps are always from UTC epoch, and calculation from given time.Time is independent of its location.
I'm happy to accept a change that removes the call to UTC in Timestamp, can you add a benchmark that demonstrates the benefit? But I'm not sure it's common knowledge that "Unix timestamp" means normalizing to UTC, so I'd prefer that bit of information to remain in the doc comment.
To me, mentioning UTC the way it currently is in the docs casts a shadow of doubt on the underlying implementation, e.g. whether Now() would (incorrectly) be somehow affected by the system time zone (which it isn't). To me, fixing this by removing that part of the comment is the most important part of this PR because it is about semantic correctness, which should be reflected in the comments.
Performance improvements are a secondary concern, but there are some benefits there, too. Benchmark added in 461ae4e602774748dc227a9e63207529813835dc.
I'd like also to point out that neither MaxTime(), Timestamp(Time), Time(uint64), or SetTime(uint64) mention UTC in any way in their docs, which is all fine. Removing the (unnecessary) mention from Now() brings it in line with these.
Distant observer here, I wanted to verify the assertions here, and after writing some code to verify for myself I can see the information is accurate. Personally, however, I also appreciate it when the docs are explicit about UTC. It's a tricky one that catches folk out.
NB. as I was writing the above tests, I noticed, Go 1.17 introduced a timestamp in milliseconds which may also simplify some of the code in this package. May also be worth thinking about if the version can be upped at all.
UnixMilli: https://pkg.go.dev/time#UnixMilli
whether Now() would (incorrectly) be somehow affected by the system time zone (which it isn't). To me, fixing this by removing that part of the comment is the most important part of this PR because it is about semantic correctness, which should be reflected in the comments.
It's important to make clear that ULID timestamps are always UTC. It's not obvious that e.g. ulid.Now will produce UTC timestamps, because it's not obvious that Unix timestamps are always UTC.
I'm fine to remove the call to UTC() in the implementation, but rather than removing the note about UTC in the docs for that one function, I think we should actually add a note about UTC to the docs of every function that produces timestamps.
it's not obvious that Unix timestamps are always UTC.
I disagree with this and the conclusion to add more redundant information to docs due to reasons stated earlier. To me, the definition of Unix time is clear about its relation to UTC.
- https://en.wikipedia.org/wiki/Unix_time
- https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_16
Anyway, rebased and removed the contentious doc tweak, I'll leave addressing the docs to someone else.