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.Format does something that is not intuitive #1508

Open
bttk opened this issue Feb 5, 2022 · 10 comments
Open

time.Format does something that is not intuitive #1508

bttk opened this issue Feb 5, 2022 · 10 comments

Comments

@bttk
Copy link

bttk commented Feb 5, 2022

I see no effective method for manipulating dates in CUE.

time.Parse helps ingest strings with time values, but it also returns a string, which needs parsing to be useful.

time.Parse(layout, value string) (string, error)

time.Format is even less useful: it can be used to create a value representing a set of all strings matching the specified layout.

func Format(value, layout string) (bool, error)

Notice that Parse and Format take arguments in different orders.

What version of CUE are you using (cue version)?

$ cue version
cue version v0.4.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you see?

-- in.cue --
import "time"

format: time.Format("2022-01-31", time.RFC3339Date)
parse: time.Parse(time.RFC3339Date, "2022-01-31")
-- log --
$ cue export in.cue
{
    "format": true,
    "parse": "2022-01-31T00:00:00Z"
}

What did you expect to see?

-- in.cue --
import "time"

format: time.Format(parse, "02/01/2006")
parse: time.Parse(time.RFC3339Date, "2022-01-31")
split: time.SplitParts(parse)
-- log --
$ cue export in.cue
{
    "format": "31/01/2022",
    "parse": "2022-01-31T00:00:00Z"
    "split": {
        "year": 2022,
        "month": 1,
        "day": 31,
        "hour": 0,
        "minute": 0,
        // ...
    }
}
@bttk bttk added NeedsInvestigation Triage Requires triage/attention labels Feb 5, 2022
@antong
Copy link
Contributor

antong commented Feb 9, 2022

I think time.Format is meant to be used to specify constraints on the format of string representations of dates and timestamps, and for that use it is quite useful and intuitive. For instance:

import "time"

#Measurement: t: time.Format(time.RFC3339)
#Measurement: value: float

data: [ ...#Measurement ]

data: [
    {t: "2022-02-09T12:34:56Z", value: 23.7 },
    {t: "2022-02-09T12:34:58Z", value: 27.8 },
    {t: "2022-02-09T12:35:00Z", value: 27.7 },
    // {t: "2022-02-32T12:36:00Z", value: 27.7 }, // -> ... time.Format: invalid time "2022-02-32T12:35:00Z"
]

There is also #319 that could help manipulating dates and timestamps. But are you looking for something like a time.FormatString() that would return a string? I don't know what the arguments would be. For it to work in a general case it would have to know about locations / time zones. JSON and CUE don't have time types so have to rely on numbers and strings. It could be an interesting thing to think about how the hermetic CUE could handle something like externally defined time zones that may change suddenly with political decisions.

@bttk
Copy link
Author

bttk commented Feb 10, 2022

Maybe time.Format just needs a different name? In its current form it is ambiguous whether "Format" is used as a noun or a verb.

The name time.FormatString(layout, value) is clear in that it is not used as a constraint.

Some ideas for the name of the constraint:

  • time.IsFormat(layout)
  • time.MatchesFormat(layout)
  • time.StringFormat(layout) /just joking/

It may be a good convention to use boolean-y IsFoo() names for constraints, but how to distinguish constraints from actual boolean values?

@mpvl
Copy link
Member

mpvl commented Feb 18, 2022

@antong @bttk
I agree the naming is unfortunate. Intuitively, Format indeed seems to imply an action.

It is a bit hard to change the names, as it would break configurations, but we are pre-v1, so we can do it with a bit of transition plan.

This is one potential path:

Note that we already have time.Time and time.Duration as validators. Note also that CUE now (still unofficially) supports variable-length args. It is not possible to use this for validators, but we plan to introduce name args (see #943), so we could support something like time.Time(format: "....") as an alternative to time.Format. The advantage of this approach is that there will be only one builtin for defining a field needs to be a time.

This is a bit ugly for boolean checks, but ultimately it is probably nicer to have separate builtins for validators and simple boolean checks, as it just reads better.

So the path would be:

Phase 1:
Introduce time.IsFormat and/or the multi-arg time.Time.
cue fix to rewrite usages of time.Format to the appropriate one.

Introduce time.FormatString to be what time.Format should be.

Phase 1b:
The use of time.Format would generate an error overridable with an env var.

Phase 2:
Officially Deprecate time.Format

Phase 3 (a few 0.x.0 versions later):
Reintroduce time.Format, now with the new meaning.
Deprecate time.FormatString.
cue fix to rewrite usages of time.FormatString to the new time.Format.

Phase 4:
Remove time.FormatString.

@mpvl
Copy link
Member

mpvl commented Feb 18, 2022

So the easiest to provide the functionality for now is to introduce time.FormatString to mean you foreseen semantics of time.Format, with the intent to transition later to time.Format.

@mpvl mpvl added Accepted and removed NeedsInvestigation Triage Requires triage/attention labels Feb 18, 2022
@mpvl
Copy link
Member

mpvl commented Feb 18, 2022

As for SplitParts, I would just simply call that Split. I think time.Split reads well.

@mpvl mpvl added the builtin label Feb 18, 2022
cueckoo pushed a commit that referenced this issue Feb 18, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Closes #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Feb 18, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Closes #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Feb 18, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Closes #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Feb 18, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Issue #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Feb 19, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Issue #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
@mpvl mpvl added this to the v0.4.3 bug fixes milestone Feb 19, 2022
@antong
Copy link
Contributor

antong commented Feb 19, 2022

I also think it would be good if there was some clearer way to directly see that, and how a function can be used as a validator. I think it is so that if the function returns bool (+ perhaps an error), or *Bottom, then you can use it as a validator by omitting the first argument in the CUE invocation. But this is not really that intuitive. When used correctly these validators look really clear and nice, but when looking at the package reference doc, at least for me it isn't until you see an example.

@mpvl
Copy link
Member

mpvl commented Feb 20, 2022

Yes, indeed, this is sorely lacking documentation. This is certainly on the todo list.

@mpvl
Copy link
Member

mpvl commented Feb 20, 2022

My plan was to document this as part of a more broader function refinement that is alluded to in #943

cueckoo pushed a commit that referenced this issue Feb 22, 2022
Down the line, FormatString should replace Format
with new semantics. It is added now as FormatString
to aid in the transition.

Issue #1508

Change-Id: I1d80aab593a7e8b18dd500d73aef12fb9e5bfa4b
Signed-off-by: Marcel van Lohuizen <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/533448
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>
@mpvl
Copy link
Member

mpvl commented Apr 4, 2022

The 0.4.3 part of this bug is fixed, moving to 0.5.x

@mpvl mpvl modified the milestones: v0.4.3 bug fixes, v0.5.x Apr 4, 2022
@myitcv myitcv added the zGarden label Jun 13, 2023
@myitcv myitcv modified the milestones: v0.5.x, v0.6.x Jun 20, 2023
@myitcv
Copy link
Member

myitcv commented Jun 20, 2023

The v0.5.x milestone is now closed because we have moved onto focus on v0.6.0. Hence moving this to v0.6.x because this didn't get done in the stretch goal that is v0.5.x.

@mvdan mvdan removed the zGarden label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants