Skip to content
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

time: provide Truncate usable for durations > 1 minute #16647

Closed
quentinmit opened this issue Aug 9, 2016 · 20 comments

Comments

Projects
None yet
6 participants
@quentinmit
Copy link
Contributor

commented Aug 9, 2016

The example for time.Time.Truncate suggests truncating by time.Hour, but in fact Truncate does not do what the user probably wants if the timezone is not hour-aligned.

e.g. https://play.golang.org/p/hAfBrYWNeZ

I suggest just removing the time.Hour example, and possibly explicitly suggest that Truncate cannot be used for times >= time.Hour because of timezones.

I doubt we can actually change the implementation of Truncate at this point, so I've marked this a Documentation bug.

@quentinmit quentinmit added this to the Go1.8 milestone Aug 9, 2016

@rakyll

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Also affecting Time.Round.

I am not satisfied we shouldn't truncate or round with anything longer or equal to one hour though. It is a perfectly valid and commonly required case.

Shouldn't we change the behavior for the non hour aligned time zones instead?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

Truncate and Round operate on the underlying time value (as the doc comments imply). They do not take the location into account.

You can truncate to the hour in the local timezone using code like https://play.golang.org/p/IWdx7kALZx (a modification of Quentin's link--it's the truncateHour function to which I am referring).

I guess the question is whether we should have a version of Truncate and Round that operate in the local timezone.

@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

Well, if you define how truncation should behave during a DST transition, I can write you a function that does it. My version already implements what I think the correct behavior should be, given that DST transitions always occur on the hour.

@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

For DST I think 01:30 EDT should truncate to 01:00 EDT and 01:30 EST should
truncate to 01:00 EST on the day when DST ends. I think your code is not
guaranteed to choose the same DST bit. (time.Date's docs say "Date returns
a time that is correct in one of the two zones involved in the transition,
but it does not guarantee which.") Then another edge case is if the
transition is not an even 60 minutes - there may not be any valid date at
all with :00 minutes.

On Aug 10, 2016 21:28, "Ian Lance Taylor" notifications@github.com wrote:

Well, if you define how truncation should behave during a DST transition,
I can write you a function that does it. My version already implements what
I think the correct behavior should be, given that DST transitions always
occur on the hour.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16647 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHEMd2euVGEcaeTQs6Oo8LkKEbDN0U5ks5qenqlgaJpZM4JgYYV
.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

There is no DST bit in the time package. There is the absolute time, and the presentation time. DST is entirely a function of the presentation time--it doesn't exist in the absolute time. The Location includes the set of times where a DST transition occurs. So whether to display DST is determined entirely by the absolute time and the Location. There is no other information.

@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2016

Of course, but whether or not there's an explicit "DST bit", it's encoded in the chosen offset.

time.Truncate as implemented always works on absolute time, but with this bug I'm trying to say that users might expect time.Truncate to operate on presentation time. Thus, we do have to care about which presentation time to show. Every time you call time.Date, in fact, you're round-tripping through presentation time (as your playground example does).

So while I would like Go to provide a time.Truncate that works usefully in presentation time, the one we have doesn't, and the documentation should explain that.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

I don't disagree with what you say, except that I think the documentation for time.Truncate does already indicate that it does not work on presentation time.

But I still don't understand why you think the truncateHour function I wrote earlier might misbehave in any scenario.

@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

But I still don't understand why you think the truncateHour function I wrote earlier might misbehave in any scenario.

Perhaps an example is best:
https://play.golang.org/p/0yAtcDwTAn

t.Truncate in this case gets things right, successive hourly times go from 01:00 EDT to 01:00 EST to 02:00 EST. Your truncateHour repeats 01:00 EDT twice, which is incorrect.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2016

Thanks for the example.

What do you think of this version?

func truncateHour(t time.Time) time.Time {
    return t.Add(-(time.Duration(t.Minute()) * time.Minute + time.Duration(t.Second()) * time.Second + time.Duration(t.Nanosecond())))
}
@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

Off the top of my head, I can't think of a timezone for which that truncateHour fails.

Now, given that, what if anything should we do? I think the docs should at least be improved to more explicitly state that Truncate does not work for time.Hour, and to remove the example. Do we want to actually change Truncate to behave as you propose? Or provide a new version of Truncate?

In general I'm concerned that, like many things related to times, truncating is subtle and easy to get wrong - as evidenced by how long it took this thread to reach a working function. Thus I would prefer if the standard library already had the right subtle behavior built-in.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

I do not think we should the existing Truncate function. I think it behaves meaningfully and as documented, though of course I'm fine with clarifying the docs. I wouldn't say it does not work with time.Hour, I would stress that it works on absolute time. And dropping or changing the example is fine with me.

If we add a new function, what would that function do? How would we document it? It's not clear that we need a variant of time.Truncate, a function that is frankly difficult to think about for any value other than time.Hour, time.Day, or some other well known value. What is the use case we are trying to implement?

@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2016

I sent CL 28730 to clarify the docs.

I think the behavior I want out of Truncate and Round is to be able to zero out the least significant fields of a time. That is, zero out the fractional seconds, or zero out the seconds, or zero out the minutes, or the hour, etc. In this particular case it came up because I want to bucket times in a way that I can then use a format string that omits the more detailed fields. For example, if I call 12:45 AM -> Truncate(Hour), I get back 12:30 AM (in some time zones). If I then go on to format that as 12 AM, that's very misleading, because it's actually the 12:30 AM - 1:30 AM bucket, not the 12 AM - 1 AM bucket.

As I pointed out with my playground link, it's not as simple as calling time.Date with some of the fields set to zero.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 7, 2016

CL https://golang.org/cl/28730 mentions this issue.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2016

I haven't been able to think of a really good solution. What if we implement a set of methods parallel to Hour and friends:

// TruncateToSecond return the time rounded down to the start of the second.
func (t Time) TruncateToSecond()

// TruncateToMinute returns the time rounded down to the start of the minute.
func (t Time) TruncateToMinute()

// TruncateToHour returns the time rounded down to the start of the hour in the time's location.
func (t Time) TruncateToHour() {}

// TruncateToDay returns the time rounded down to the start of the day in the time's location.
func (t Time) TruncateToDay() {}

// TruncateToMonth returns the time rounded down to the start of the month in the time's location.
func (t Time) TruncateToMonth() {}

// TruncateToYear returns the time rounded down to the start of the year in the time's location.
func (t Time) TruncateToYear() {}

gopherbot pushed a commit that referenced this issue Sep 12, 2016

time: improve Truncate and Round documentation
Truncate and Round operate on absolute time, which means that
Truncate(Hour) may return a time with non-zero Minute(). Document that
more clearly, and remove the misleading example which suggests it is
safe.

Updates #16647

Change-Id: I930584ca030dd12849195d45e49ed2fb74e0c9ac
Reviewed-on: https://go-review.googlesource.com/28730
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@quentinmit

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

@ianlancetaylor Hmm, that's a pretty significant API surface increase, for a type that already has a bunch of methods. Especially if you also want to offer Round varieties.

We could do something like t.TruncateTo().Hour() to hide everything under another type, though maybe that's just synctatic sugar. Or Truncater(t).Hour() with a Truncater type that implements those methods?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

We could make them functions rather than methods.

@minux

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

@rakyll

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

I don't agree that the truncating by day is a utility but a correct implementation of a specific case for Truncate. It should be next to Truncate rather than being in a util package.

@quentinmit quentinmit changed the title time: Truncate example invalid time: provide Truncate usable for durations > 1 minute Oct 5, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

We're not going to add new API here.
People who want to truncate to hour can build that with Date.
The doc comments have already been updated to warn against
Truncate(Hour). That's enough.

@rsc rsc closed this Oct 18, 2016

DmitriyMV pushed a commit to DmitriyMV/todolist that referenced this issue Mar 2, 2017

DmitriyMV
This commit removes all usages of github.com/jinzhu/now.
github.com/jinzhu/now has numerous errors handling date and time. The example's of them are:
1. Incorrect handling of DST (jinzhu/now#13).
2. 24 Hour long days (https://stackoverflow.com/questions/25254443/return-local-beginning-of-day-time-object-in-go) Chicago has 23, 24, or 25 hours in a day.
3. Incorrect usage of time.Truncate which can return time with a non-zero minute, depending on the time's Location. (golang/go#16647).

There are other issues as well, including getting incorrect beginning and end of the month.

DmitriyMV pushed a commit to DmitriyMV/todolist that referenced this issue Mar 2, 2017

DmitriyMV
This commit removes all usages of github.com/jinzhu/now.
github.com/jinzhu/now has numerous errors handling date and time. The example's of them are:
1. Incorrect handling of DST (jinzhu/now#13).
2. 24 Hour long days (https://stackoverflow.com/questions/25254443/return-local-beginning-of-day-time-object-in-go) Chicago has 23, 24, or 25 hours in a day.
3. Incorrect usage of time.Truncate which can return time with a non-zero minute, depending on the time's Location. (golang/go#16647).

There are other issues as well, including getting incorrect beginning and end of the month.

DmitriyMV pushed a commit to DmitriyMV/todolist that referenced this issue Mar 4, 2017

DmitriyMV
This commit removes all usages of github.com/jinzhu/now.
github.com/jinzhu/now has numerous errors handling date and time. The example's of them are:
1. Incorrect handling of DST (jinzhu/now#13).
2. 24 Hour long days (https://stackoverflow.com/questions/25254443/return-local-beginning-of-day-time-object-in-go) Chicago has 23, 24, or 25 hours in a day.
3. Incorrect usage of time.Truncate which can return time with a non-zero minute, depending on the time's Location. (golang/go#16647).

There are other issues as well, including getting incorrect beginning and end of the month.

@golang golang locked and limited conversation to collaborators Oct 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.