time: add time module #327

b5 · 2020-12-04T17:39:56Z

Signed-off-by: b5 sparkle_pony_2000@qri.io

A few notes:

I've added this to lib/time based on @alandonovan's feedback from Time and duration module #326
we're waiting on a chat with @srebhan about the naming of the constructors vs. parsing functions before this is final
We use a tool to extract documentation from this package in doc.go into other formats. I can remove this to conform to other packages, but if this info can stay & be maintained, I'd be deeply appreciated. Keeping documentation to the code itself has made our lives much easier.
If we merge this I'll take this back to our contributors on the starlib project & chat about relying on this package upstream. @alandonovan in the past you'd mentioned interest in other packages from the project, now might be the time to go shopping 😉

google-cla · 2020-12-04T17:40:13Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

b5 · 2020-12-04T18:08:37Z

@googlebot I signed it!

google-cla · 2020-12-04T18:08:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

alandonovan

Just a first pass. Thanks for contributing!

lib/time/time.go

srebhan · 2020-12-07T15:25:24Z

@b5: Thanks for your quick reaction! I have duration() and time() as "constructors" and use parse_{time,duration}() as parsing function. I think having the "class-name" as constructors is a matter of least-surprise and the the name for the parsing functions are obvious. However, I see that this would crush your current API...
Is there anything I can do to speed this up?

@alandonovan: I'm supporting this PR and will close mine once this one is merged.

google-cla · 2020-12-09T17:47:16Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

lib/time/time.go

srebhan · 2020-12-16T17:34:50Z

@b5 any news on this one? I'm eagerly waiting for time support! ;-P

google-cla · 2020-12-18T17:21:40Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

b5 · 2020-12-18T17:42:25Z

hi all,

I still have a little more cleanup to do here, but wanted to get this set of changes up for review. I've made a bunch of adjustments to the Starlark API based on review feedback.

@srebhan, what do you think the signature for the time.time constructor should be? I've currently got it wrapping golang's time.Date, but could also see removing time.from_timestamp and using that instead.

srebhan · 2020-12-19T15:03:10Z

@b5 I had time([year[, month[, day[, hour[, minute[, second[, nanosecond[, location]]]]]]]]) to mimic the python API a bit, but I think a kwargs approach also works. Whatever you prefer to be honest! :-)

lib/time/doc.go

lib/time/time.go

srebhan · 2020-12-20T16:15:51Z

lib/time/time.go

+func parseLocation(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	var s string
+	if err := starlark.UnpackArgs("location", args, kwargs, "s", &s); err != nil {
+		return nil, err
+	}
+	loc, err := time.LoadLocation(s)
+	if err != nil {
+		return nil, err
+	}
+
+	return starlark.String(loc.String()), nil
+}


I don't get the use of this function... You put in a location string and get that string out? Why do you need this?

this validates system clock time zone locations, users can use it to both confirm the timezone in question is supported by the OS. In the future This could be expanded to return a starlark "location" type that supports timezone conversion

Can we then please name the function validate_location() or similar and return a bool!? Otherwise it is very confusing.

Starlark's error-handling theory is not well developed. It doesn't have exceptions, and I don't think we really want to add them, but this does mean that callers need to "look before they leap", or need a mode in which they succeed but return an error value (e.g. None or perhaps a string) instead of failing.

@alandonovan right, but parseLocation suggests that we somehow convert the given location to something else. Instead we simply return the same string in case of success or nil otherwise. Furthermore, the returned error is totally redundant with checking the string.... My suggestion is to simplify this to return a bool indicating if this is a valid location. So instead of

l, err := parseLocation(..., loc) if err != nil {...}

you can call

if isValidLocation(..., loc) {...}

srebhan · 2020-12-20T16:19:35Z

lib/time/time.go

+		return starlark.None, err
+	}
+
+	SleepFunc(time.Duration(dur))


You need to check SleepFunc being not nil otherwise we might segfault here.

Personally I think that if users set SleepFunc to nil and try to call SleepFunc the program should panic.

Well I don't like panicking on this level. Imagine you expose setting the sleep function to the user script (for whatever reason). Now your whole program will explode without any way for the programmer to intervene. IMO we should either throw an error here or use the default sleep function in presence of SleepFunc being nil.

I agree with @b5 that it's fine to panic here, as it's an application programming error.

Ok, but this means we never should expose setting SleepFunc to the interpreter and thus to the user. To be honest I would make SleepFunc == nil the default and then run time.Sleep to circumvent all of this, but it's not my decision. :-)

It's fine to insist that the value be non-nil, and simply panic if someone violates that. Users: don't do that.

srebhan · 2020-12-20T16:20:21Z

lib/time/time.go

+}
+
+func now(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	return Time(NowFunc()), nil


We need to check NowFunc for not being nil to avoid a segfault here.

FYI NowFunc cannot return nil because it is actually time.Now

It can be nil if users override the package variable, setting time.NowFunc = nil. In discussion above Alan & I have agreed it's the package consumer's responsibility to not do that

@b5 thanks for the info, if I get it right, it is acceptable to let if fail then right (leave it as it is now)?

yep! I think it should be left as is

lib/time/time.go

essobedo · 2021-02-19T18:34:08Z

@b5 I've just removed it

essobedo · 2021-02-20T14:31:45Z

FYI I renamed the package to starlarktime to remain consistent with other modules like the json module

adonovan · 2021-02-20T19:18:13Z

Sorry for radio silence: I'm on vacation at the moment but will attend to this PR fully on Monday.

…

On Sat, 20 Feb 2021 at 09:32, Nicolas Filotto ***@***.***> wrote: FYI I renamed the package to starlarktime to remain consistent with other modules like the json module — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#327 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLFMP37RNWFAHJWNEUPBBDS77BWPANCNFSM4UNXYUDQ> .

cmd/starlark/starlark.go

alandonovan · 2021-02-22T18:54:24Z

starlark/testdata/time.star

+assert.eq(10*60*60*1000000, d2.microseconds)
+assert.eq(10*60*60*1000000000, d2.nanoseconds)
+
+# duration == duration = boolean


Remove = boolean here and below

starlark/testdata/time.star

starlarktime/time.go

alandonovan · 2021-02-22T19:51:04Z

starlarktime/time.go

+			return Time(x.Add(time.Duration(-y))), nil
+		case Time:
+			// time - time = duration
+			if side == starlark.Left {


This condition is (I think) always true, because the left operand is always tested for HasBinary first, so the "else" case is unreachable. Simplify.

alandonovan · 2021-02-22T19:58:26Z

starlarktime/time.go

+	return names
+}
+
+// threeway interprets a three-way comparison value cmp (-1, 0, +1)


FYI: in hindsight, the API design that required threeway was a mistake. Originally it was intended to support floating point NaNs according to IEEE 754, which requires that x < NaN, Nan < x and NaN == x are all false, so you can't represent the result of a "float op float" comparison using a three-valued result (smaller, bigger, equal): one must also know the query operator. But we changed things recently so that floats are totally ordered (NaN > +Inf), otherwise you can't sort floats, so CompareSameType could in principle be simplified to:

CompareSameType(v Value, depth int) (signum int, err error)

alandonovan · 2021-02-22T19:59:49Z

starlarktime/time.go

@@ -0,0 +1,507 @@
+package starlarktime


What happened to the doc comment?

@alandonovan it has been removed since it was not in a standard format and there is no other module with this kind of documentation

essobedo · 2021-02-23T19:43:55Z

@alandonovan Remarks applied, please check again

ssoroka · 2021-03-03T19:49:48Z

Any update on this? :)

essobedo · 2021-03-04T07:44:55Z

@b5 I cannot resolve conversations, could you please check if it is possible to give me the necessary rights?

essobedo · 2021-03-04T14:23:40Z

@adonovan @alandonovan We really need this feature for https://github.com/influxdata/telegraf, so it is important for us to finish it and have it in Startlark. Can you still do the review or shall I ask someone else? and if so, who is the new project lead now please? (cc @ssoroka @srebhan)

adonovan

Sorry for the delay. I'll do what I can to help you get this committed today.

adonovan · 2021-03-04T14:25:22Z

lib/time/time.go

+		"parse_location": starlark.NewBuiltin("parse_location", parseLocation),
+		"now":            starlark.NewBuiltin("now", now),
+		"time":           starlark.NewBuiltin("time", newTime),
+		"parse_time":     starlark.NewBuiltin("time", parseTime),


NewBuiltin("parse_time")

adonovan · 2021-03-04T14:32:09Z

lib/time/time.go

+
+func parseLocation(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	var s string
+	if err := starlark.UnpackPositionalArgs("location", args, kwargs, 1, &s); err != nil {


"parse_location" (for consistency)

Though I agree with earlier remarks that "parse" is the wrong verb: it's really "validate" or "canonicalize". Where's the Starlark documentation for this function? Does it ever return a value different from its argument? The Go documentation for LoadLocation suggests the answer is: yes, but only when s is "" (an alias for "UTC"), or "Local", in which case it uses local time: but this doesn't seem like a concept we want to expose to Starlark since it breaks determinism.

I would be inclined to disallow both of these, which would then allow us to make this function be: is_valid_timezone(string) bool.

In any case, we need to spell out the intended behavior of all the functions in this file.

adonovan · 2021-03-04T14:35:13Z

lib/time/time.go

+	case syntax.SLASH:
+		switch y := y.(type) {
+		case Duration:
+			if int64(y) == 0 {


adonovan · 2021-03-04T14:35:54Z

lib/time/time.go

+			if int64(y) == 0 {
+				return nil, fmt.Errorf("%s division by zero", d.Type())
+			}
+			return starlark.Float(float64(x.Nanoseconds()) / float64(time.Duration(y).Nanoseconds())), nil


starlark.Float(x.Nanoseconds()) / starlark.Float(time.Duration(y).Nanoseconds())

adonovan · 2021-03-04T14:36:53Z

lib/time/time.go

+			if !ok {
+				return nil, fmt.Errorf("int value out of range (want signed 64-bit value)")
+			}
+			if int64(i) == 0 {


adonovan · 2021-03-04T14:48:20Z

lib/time/time.go

+			if int64(i) == 0 {
+				return nil, fmt.Errorf("%s division by zero", d.Type())
+			}
+			return Duration(x.Nanoseconds() / i), nil


Simpler: x / Duration(i)

(This is what you used for //, and the behavior is identical.)

@adonovan you meant d / Duration(i), right?

Yes, quite right.

lib/time/time.go

adonovan · 2021-03-04T15:08:57Z

lib/time/time.go

+func (d Duration) CompareSameType(op syntax.Token, v starlark.Value, depth int) (bool, error) {
+	x := time.Duration(d)
+	y := time.Duration(v.(Duration))
+	switch op {


return threeway(op, x, y), nil

@adonovan I'm not sure, I get what you expect here. You would to see the content of the switch block moved into a new private function?

Yes, and that function already exists below: it's threeway.

@adonovan I probably miss something obvious but the current function has only 2 parameters while you are trying to call a function with 3 parameters. At best the second parameter could be somehow x-y but we could get an overflow.

Oops, you're right; I'm not really using my brain. What I mean was this, which is less concise that I had imagined:

cmp := 0 if x, y := d, v.(Duration); x < y { cmp = -1 } else if x > y { cmp = 1 } return threeway(op, cmp)

essobedo · 2021-03-04T21:52:16Z

@adonovan Thx for the feedbacks, it is hard to follow it properly since I have no way to resolve the discussions but I think that I addressed your feedbacks, could you please check it again?

adonovan

LGTM. Thanks for your patience!

adonovan · 2021-03-04T22:09:48Z

lib/time/time.go

+// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
+func parseDuration(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	var d Duration
+	err := starlark.UnpackPositionalArgs("duration", args, kwargs, 1, &d)


"parse_duration"

@alandonovan indeed, fixed

b5 · 2021-03-05T12:32:38Z

quick think you to everyone who got this off the ground! @srebhan for motivating this in the first place, and @essobedo for putting in a huge amount of work to implement @adonovan's careful review 😄

srebhan · 2021-03-05T13:06:54Z

@b5 I also want to thank you for your continuous effort in bringing this forward. I know it's not easy to find the time. Special thanks to @essobedo for investing so much and pushing this over the finishing line!

essobedo · 2021-03-05T14:56:19Z

@adonovan Thanks for the impressive review, can you still somehow merge ? or do you at least know who can merge?

…am versions After a *large* community effort, we've successfully moved two packages from starlib into the "real" standard library in go-starlark: * math: google/starlark-go#357 * time: google/starlark-go#327 Both of these packages have had a great deal of vetting & improvement in the process of migrating. While this does introduce many breaking changes for both packages, both packages have better stability & performance characteristics. Moving forward, we'll try to follow the pattern of developing & testing packages here, and for those deemed worthy, move them up into go-starlark. Once a package lands there, we'll switch starlib to being a strict import-and-document-only, which effectively locks their API. Keeping package APIs locked will cut down on drift, benefitting the broader starlark ecosystem. BREAKING CHANGE: math & time modules have been overhauled. Refer to package documentation for details

…am versions After a *large* community effort, we've successfully moved two packages from starlib into the "real" standard library in go-starlark: * math: google/starlark-go#357 * time: google/starlark-go#327 In addition, both time & math imports have lost thier ".star" suffixes: load("time.star", "time") -> load("time", "time") load("math.star, "math") -> load("math", "math") Both of these packages have had a great deal of vetting & improvement in the process of migrating. While this does introduce many breaking changes for both packages, both packages have better stability & performance characteristics. Moving forward, we'll try to follow the pattern of developing & testing packages here, and for those deemed worthy, move them up into go-starlark. Once a package lands there, we'll switch starlib to being a strict import-and-document-only, which effectively locks their API. Keeping package APIs locked will cut down on drift, benefitting the broader starlark ecosystem. BREAKING CHANGE: math & time modules have been overhauled. Refer to package documentation for details

b5 mentioned this pull request Dec 4, 2020

Time and duration module #326

Closed

alandonovan reviewed Dec 4, 2020

View reviewed changes

srebhan reviewed Dec 10, 2020

View reviewed changes

lib/time/time.go Outdated Show resolved Hide resolved

srebhan reviewed Dec 10, 2020

View reviewed changes

lib/time/time.go Outdated Show resolved Hide resolved

srebhan reviewed Dec 10, 2020

View reviewed changes

lib/time/time.go Outdated Show resolved Hide resolved

alandonovan reviewed Dec 10, 2020

View reviewed changes

b5 force-pushed the feat_time_module branch from f88a7d0 to adf86ee Compare December 18, 2020 17:21