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

proposal: time: Duration should have .Ceil method #39726

Closed
JAicewizard opened this issue Jun 20, 2020 · 13 comments
Closed

proposal: time: Duration should have .Ceil method #39726

JAicewizard opened this issue Jun 20, 2020 · 13 comments

Comments

@JAicewizard
Copy link

What version of Go are you using (go version)?

$ go version
go1.14.4

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jaap/.cache/go-build"
GOENV="/home/jaap/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jaap/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build626039516=/tmp/go-build -gno-record-gcc-switches"

Having a ceil method is usefull for cases where you have a minimum duration, but in reality it needs to be on a per X basis, so the duration might only be 0.5 seconds, but in-reality it can only finish once aa second so you want to round it up to 1 second.
The Time package provides a time round method for rounding to the nearest X and a truncate method for rounding down(towards 0) but not the other way around (away from 0) making it incomplete.

@odeke-em odeke-em changed the title time.Duration should have .Ceil method proposal: time: Duration should have .Ceil method Jun 22, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 22, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 7, 2020
@mdempsky
Copy link
Member

Having a ceil method is usefull for cases where you have a minimum duration, but in reality it needs to be on a per X basis, so the duration might only be 0.5 seconds, but in-reality it can only finish once aa second so you want to round it up to 1 second.

Can you elaborate on this use case? I'm having a hard time envisioning what exactly you mean.

@rsc rsc moved this from Incoming to Active in Proposals (old) Aug 12, 2020
@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

@JAicewizard can you respond to @mdempsky's question? I too am having a little bit of trouble understanding why this is important to add to the API.

@kaey
Copy link

kaey commented Aug 27, 2020

I believe OP wants to replace this code with a method

dur := durationOfSomeTask
X := time.Second
if dur < X {
    dur = X
}

@JAicewizard
Copy link
Author

Imagine you have a task taking X duration and you have timeslots of length Y and task X can only finish at the end of a timeslot.
At the moment I have:

startTime = {start time} //time.Time
X := {duration} //time.Duration
realDuration := X.Truncate(Y)
endtime = start.Add(realDuration)
//realDuration will be Y less then we need, unless X == n*Y
if realDuration != X{
    endtime.Add(Y)
}

This would all be solved by being able to round up to the neared multiple of Y instead of just being able to round down/nearest.

@rsc
Copy link
Contributor

rsc commented Aug 28, 2020

I'm sorry but I still don't understand. It still seems like very hypothetical. Can you give a more concrete reason why this would come up in a real Go program?

Note that endtime := start.Add((X+Y-1).Truncate(Y)) works today.

@JAicewizard
Copy link
Author

This is a real world example of where this is usefull. lesson.startTime/endTime are just timestamps and their week/year is irrelevant.
min/max are the start/end time these lessons should be active for (like term begin/end dates for example)

Here we calculate how much these lesson.startTime/endTime should be offset by in order to be the first occurance of this timerange withing this term start/end time range.
This is done by calculating how much it should be offset by to match the start time, and then round up to 1 week. Or in this case round down and then optionally add 1.

timeOff := min.Sub(lesson.startTime).Truncate(time.Hour * 24 * 7)
lesson.startTime = lesson.startTime.Add(timeOff)
lesson.endTime = lesson.endTime.Add(timeOff)

//remember, we truncate, so we will almost always fall short 1 week
if lesson.startTime.Before(min) {
	lesson.startTime = lesson.startTime.Add(time.Hour * 24 * 7)
	lesson.endTime = lesson.endTime.Add(time.Hour * 24 * 7)
}

This is an example of a real world program, or well if it ware not scraped it would be running in the real world.
Particular code is part of importing data from a scheduling program.

You are right that endtime := start.Add((X+Y-1).Truncate(Y)) might do the exact same, but it is unintuitive. .Ceil is much simpler to read, write and reason about when making changes.

@rsc
Copy link
Contributor

rsc commented Aug 28, 2020

OK, I think I understand what you are trying to write. Thanks for bearing with me. I wrote a running demo at https://play.golang.org/p/GUreZdb5pYI:

package main

import (
	"fmt"
	"time"
)

func main() {
	start := time.Date(2020, 8, 1, 0, 0, 0, 0, time.Local)
	end := time.Date(2020, 8, 31, 23, 59, 59, 0, time.Local)

	fmt.Println(start)
	fmt.Println(end)
	fmt.Println()

	example := time.Date(2001, 1, 1, 14, 30, 0, 0, time.Local)

	const D = 24 * 7 * time.Hour
	first := example.Add((start.Sub(example) + D - 1).Truncate(D))
	// want to write: first := example.Add(start.Sub(example).Ceil(D))

	fmt.Println(example)
	fmt.Println("=>")
	fmt.Println(first)
}

Note that while it works on the playground in UTC, it actually fails on my local system, in that the time of the event changes from 14:30 to 15:30 because of a daylight savings change. Not all weeks are 247time.Hour long.

Note also that (d+D-1).Truncate(D) is not even correct when d < 0, so if we change example to be the year 2100, first will end up being August 14 instead of August 7. (d+D-1).Floor(d) would be correct, but we don't have Floor, only Truncate.

It would be weird to have Ceil and Truncate but not Floor, so really this proposal would be to add both Ceil and Floor.

And I'm also not convinced Ceil and Floor would be the right names since those are so tied to real -> integer conversions. It would be better to call them RoundUp and RoundDown. So Duration would have Truncate, Round, RoundUp, and RoundDown.

It's not too bad to consider adding those two extra methods. They are simple enough.

But since every new method incurs cost for users reading docs and learning the APIs, I'd still like to understand whether there's a widespread need, ideally in an example that doesn't depend on daylight savings time not happening.

I looked at C#, Java, and Python to see if there was evidence of need in other languages, and as far as I can tell those all don't have any kind of rounding methods whatsoever.

@JAicewizard
Copy link
Author

I dont know if there is a widespread need, but considering there is a Truncate clearly there used to be a need of some kind of rounding. I found it odd to only see Truncate and not the other round methods since they are usually provided together.
Maybe it is also worth considering adding the opposite of Truncate, I dont know what it would be called but with those 2 combined users can easily construct Floor and Ceil. This might not be as elegant but if keeping the API tight is the goal it could be an option.

Note that while it works on the playground in UTC, it actually fails on my local system, in that the time of the event changes from 14:30 to 15:30 because of a daylight savings change. Not all weeks are 24_7_time.Hour long.

Wouldnt daylight savings be a new timezone (in the go timezone sense)? ₣om what I can see a go timezone is always the same offset to GMT.

@ianlancetaylor
Copy link
Contributor

Go timezones do not have a fixed offset from GMT. They do support daylight savings time. As the docs say: "For many Locations the time offset varies depending on whether daylight savings time is in use at the time instant."

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

If there were other use cases for RoundUp/RoundDown that come up the future, then it seems like it would be fine to revisit this. But this use case doesn't seem like it justifies them, since the use does not result in a correct program.

Are there any other use cases? If not, we should probably decline this.

@JAicewizard
Copy link
Author

I dont know, maybe there are, maybe not.
I would definitly think of adding the opposite of .Truncate aka round away from 0, just to make it symmetrical.

If this is closed, I think we should invite people to reopen if they find a good use-case for this.

@rsc
Copy link
Contributor

rsc commented Sep 16, 2020

Based on the discussion above, this seems like a likely decline.
It would be fine to file a new proposal for RoundUp/RoundDown, mentioning this one, if compelling use cases can be found.
(The use case here does not quite work.)

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Sep 16, 2020
@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Sep 23, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Sep 23, 2020
@golang golang locked and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants