Fix Comment on the the Timestamp.Time() method #197
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #197 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 545 545
=========================================
Hits 545 545 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
I thought of one situation in which this could be a breaking change. From the documentation for
So if anyone was relying on the previous behavior of being Is this something we need to worry about? |
kohenkatz
dismissed
their stale review
I think this might be considered a breaking change
|
Maybe you can do something like that: // Time returns the time.Time representation of a Timestamp.
// Deprecated: use TimeUTC instead.
func (t Timestamp) Time() (time.Time, error) {
secs := uint64(t) / _100nsPerSecond
nsecs := 100 * (uint64(t) % _100nsPerSecond)
return time.Unix(int64(secs)-(epochStart/_100nsPerSecond), int64(nsecs)), nil
}
// TimeUTC returns the UTC time.Time representation of a Timestamp.
func (t Timestamp) TimeUTC() (time.Time, error) {
base, err := t.Time()
if err != nil {
return time.Time{}, err
}
return base.UTC(), nil
}This avoids any side effects or breaking changes. The other safest approach is to only update the documentation: // Time returns the local time.Time representation of a Timestamp.No possible regression but the existing "bug" will still be here if someone created its code based on the comment. |
EDIT: This is expected behavior. Please read later replies (#197 (comment)) for explanation of behaviorFrom my very small perspective in my very small corner of the world: The current behavior is broken, both in theory and practice. In practice: func TestTimeZone(t *testing.T) {
uuids := []UUID{
Must(FromString("00000000-0000-7000-8000-000000000000")),
Must(FromString("10000000-0000-7000-8000-000000000000")),
Must(FromString("20000000-0000-7000-8000-000000000000")),
Must(FromString("30000000-0000-7000-8000-000000000000")),
Must(FromString("30000000-0000-7000-8000-000000000000")),
Must(FromString("40000000-0000-7000-8000-000000000000")),
Must(FromString("50000000-0000-7000-8000-000000000000")),
Must(FromString("60000000-0000-7000-8000-000000000000")),
Must(FromString("70000000-0000-7000-8000-000000000000")),
Must(FromString("80000000-0000-7000-8000-000000000000")),
Must(FromString("90000000-0000-7000-8000-000000000000")),
Must(FromString("a0000000-0000-7000-8000-000000000000")),
Must(FromString("b0000000-0000-7000-8000-000000000000")),
Must(FromString("c0000000-0000-7000-8000-000000000000")),
Must(FromString("d0000000-0000-7000-8000-000000000000")),
Must(FromString("e0000000-0000-7000-8000-000000000000")),
Must(FromString("f0000000-0000-7000-8000-000000000000")),
}
for _, uid := range uuids {
t.Run(uid.String(), func(t *testing.T) {
got, _ := TimestampFromV7(uid)
tm, err := got.Time()
t.Log(tm.Zone())
if err != nil {
t.Fatalf("uuid.Time(%q) failed: %v", uid, err)
}
})
}
}Output (at least for me) gives four time zones (CST,CWT,LMT,CDT): In theory: As your quote from the time package points out:
The code currently does none of those (.UTC() or.Local(), and .Round(0)). So we're sure that the monotonic clock is changing regarding map keys. (A caller would have to be doing that themself, already.) And regarding equals and a timezone the time package provides a .Equal() method intentionally specifying the following:
Additionally, this is memory-internal. Unless the caller is doing string or binary-equivalent comparison on a previously saved time.Time object E.g. intentionally saving the timezone off somewhere, then bringing it back and comparing, this is a non-issue. If only in memory, then when the process is restarted after updating this package, all the comparisons should work as usual. If they saved it off as a string or binary representation, that has no time zone data, either, so marshaling it back to a UUID should be fine. Only if someone is calling the .Time() method and then saving off some representation of it with timezone that persists process restarts or communicates with a system that includes the timezone, but is not actually parsing the timezone would this be an issue. (I think.... maybe I'm missing a corner case somewhere.) |
This is actually working as expected. The four time zone abbreviations you are seeing are an artifact of dealing with past timestamps.
Your sample times are missing one other option in your area: "CPT" = "Central Peace Time" (after the conclusion of WWII). The reason you are seeing this in an unexpected order ("LMT" seen after Standard time) is because of your other issue, #195, which is causing the dates to be calculated incorrectly. Here is what I get (US Eastern time) if I add Note the use of "EWT" for 1942, "EST/EDT" for everything else after 1883 (the creation of "Railway Time" which became "Standard Time", and "LMT" for everything before that.
Not explicitly, but it does the same thing in a not-well-documented way. Calls to
The case you are missing here is exactly what is mentioned in the documentation: |
Well, now I learned something today! I'm calling that a good day.
Thanks for pointing that out. My quick read didn't catch that.
Yes, that makes sense. I pity the person attempting to wrangle a Local timezone as a map key. That sounds unpleasant. |
I like either of these.... why does Time() return an error? (I'll assume legacy reasons.) |
Not exactly for legacy reasons. It was left there "for potential unknown cases in future to avoid [breaking changes]". 😐 See #31 (comment) The original PR on this project's predecessor does have a use for it, but that use was never relevant in this implementation. |
|
Happy to update this PR. Is there a preference from the new function and deprecated function approach vs just updating the comment? |
|
Personally, I would go with just updating the comment. Anyone who wants to call |
nathanmcgarvey-modopayments
changed the title
There was a problem hiding this comment.
I'm not sure this is better. For those of us who understand the construction of time.Time values from Unix timestamps, the new meaning is clear.
But I think that anyone who does not understand that time.Unix() uses the Local time zone might think that this means that we are counting the Unix timestamp from the local time zone's 1970-01-01 00:00:00 instead of from UTC.
How about this?
// Time returns the time.Time representation of a Timestamp.
//
// For display and comparison purposes, note that this function produces a time.Time value that contains only wall clock time (no monotonic counter) and uses the time.Local time zone for display.
It simplifies the basic description, and puts the details further down for anyone who cares.
|
Counter offer (slightly reworded and formatted): |
That looks good to me |
…mestamp.Time method and revert to original behavior
nathanmcgarvey-modopayments
force-pushed
the
fix_time_to_be_UTC
branch
from
9f69da1 to
de3c87c
Compare
Fixes #196