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: add Time.GoString method to help %#v #39034

Open
kevinburkemeter opened this issue May 13, 2020 · 17 comments
Open

time: add Time.GoString method to help %#v #39034

kevinburkemeter opened this issue May 13, 2020 · 17 comments

Comments

@kevinburkemeter
Copy link

@kevinburkemeter kevinburkemeter commented May 13, 2020

I have an array of time.Time objects that I retrieved from the database, and I would like to place them in a Go source file. As I understand it, there's not an easy way to do this without a helper method.

  • fmt.Sprintf("%#v\n", t) prints out something like:

    time.Time{wall:0x0, ext:63724924180, loc:(*time.Location)(nil)}

    which not only makes it difficult to determine what time is represented - I can't do math on 63724924180 in my head to determine whether this is yesterday or today, but also can't be embedded directly in a Go program because wall and ext and loc are private variables.

  • I could use t.Format(time.RFC3339) on each item to get an array of strings, but then these need to be re-Parse'd to get a time.Time back, with appropriate error handling.

It could be nice to have either a (time.Time) GoString() string method or a GoString format constant which would print out a format that could be embedded in a Go source file.

For example

fmt.Sprintf(t.Format(time.GoString))

could yield

"time.Date(2020, time.February, 31, 23, 59, 59, 0, nil)"

or similar. I am not sure whether having time.Time implement the GoStringer interface - and change the format printed by %#v - would violate the Go compatibility promise, though I doubt the current wall, ext, loc format is very useful for most people.

The trickiest part of this would be printing Location, which could be implemented by printing out the pointer address for non-nil, non-UTC, non-Local locations - it's no worse than what's currently done.

I suppose we'd also lose some data about the monotonic-ness of the time measurement in question but if you're trying to put this in source code you likely don't care about that bit.

@gopherbot gopherbot added this to the Proposal milestone May 13, 2020
@gopherbot gopherbot added the Proposal label May 13, 2020
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented May 13, 2020

Ah, t.Format(time.GoString) would not work since we can only do string formatting there and that would likely be insufficient for printing the Location part so it does not need error handling. Still we could satisfy the GoStringer interface.

@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented May 13, 2020

I'm not really sure that needs to be in the stdlib, it's trivial to implement without t.Format

https://play.golang.org/p/Lzzbd55VBeH

@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented May 13, 2020

If your struct is

type A struct {
     B time.Time
}

a := &A{B: time.Now()}
fmt.Printf("%#v\n", a)

Suddenly you need to either implement your custom format string for everything, just so you can get at the time.Time, or hack up something like Printf("&A{B: %s}\n", toGoTime(a.B)) to rebuild the struct formatting around your custom element formatting.

More to the point this is why the GoStringer interface exists.

@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented May 13, 2020

I see your point, you can still easily wrap it but yeah it'd be nice.

https://play.golang.org/p/OhAGAR6bPs4

@rsc rsc changed the title proposal: easier formatting of time.Time for placement in Go source code proposal: time: add Time.GoString method to help %#v Jun 10, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

This mostly makes sense, but it would have to hide the monotonic time if any.

@rsc rsc added this to Incoming in Proposals Jun 10, 2020
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Oct 17, 2020

Hi, ping here, I think this would be a very helpful change for Go 1.16, and I would love to work on this if there is interest.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2020

Change https://golang.org/cl/267017 mentions this issue: time: add GoString method

@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Feb 24, 2021

Hi, now that the 1.17 source tree is open, I would be interested in getting feedback on this proposal. I submitted CL 267017 as an idea for how this feature could be implemented, that could hopefully be used as a jumping off point.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

This seems reasonable (still).
Will list in the minutes this week.

Does anyone object to adding this?

@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Feb 24, 2021
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Feb 24, 2021

The stickiest point I think is how to represent a *time.Location value. The most common cases for a *time.Location - nil, UTC, or Local - are all easy to handle. However the other cases - fixed zones, or Locations that represent values in the timezone database, like "America/Los_Angeles" - are more tricky.

  1. One way would be to try to represent it the same way we represent nested pointers in a struct.

    (*time.Location)(0xbadbcde)

    As I understand the runtime, it would be difficult to get the pointer address without importing either fmt (which imports reflect) or reflect directly, to get and then print the pointer address. The time package is not allowed to import reflect as Russ pointed out.

    I suppose we could special case time.Time inside of the fmt package when someone asks for a GoString representation, since fmt is allowed to import reflect, but that seems less than ideal.

  2. Another way would be to convert it to a fixed zone, with the offset determined by the instant represented by the time. You lose information here in some cases, because you would be converting something like "America/Los_Angeles" into a fixed time zone with fixed offset.

    time.FixedZone(loc.name, determineOffset(loc))

    If loc is the America/Los_Angeles timezone, determineOffset would return 8*60*60 = 28800 for a time instant in the winter, and 7*60*60 = 25200 for a time instant in the summer when daylight savings time is active.

  3. Another way would just be to print a time.Location value with a lowercase name property. This wouldn't compile, but would be no worse than what we print today for %#v, which also does not compile. We'd need to build in logic to escape strings in case the name field has a " or other characters in it that need escaping.

    time.Date(2020, time.February, 31, 23, 59, 59, 0, &time.Location{name: "America/Los_Angeles"}) 
    ./prog.go:9:79: cannot refer to unexported field 'name' in struct literal of type time.Location
  4. Another way would be to replace it with time.LoadLocation, which would also not compile because LoadLocation returns two values. I don't think this is a great idea, but thought I'd mention in the interest of completeness.

    time.Date(2020, time.February, 31, 23, 59, 59, 0, time.LoadLocation("America/Los_Angeles"))
    ./prog.go:9:81: multiple-value time.LoadLocation() in single-value context

Of these, I think (3) is best. I don't think it's possible to implement (1) unless I am missing some aspect of the runtime that would make it possible to retrieve a pointer without importing the reflect package.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

GoString doesn't have to give exact Go syntax, I don't think. It prints things like 'main.T', which is not going to compile either. So just saying time.Location("NAME") would probably be fine.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

To be clear, I believe the current proposal is to return strings like:

time.Date(2020, time.February, 31, 23, 59, 59, 0, time.UTC)
time.Date(2020, time.February, 31, 23, 59, 59, 0, time.Location("Americas/New_York")
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Mar 24, 2021

Yes, that's my understanding

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 24, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: time: add Time.GoString method to help %#v time: add Time.GoString method to help %#v Mar 24, 2021
@rsc rsc modified the milestones: Proposal, Backlog Mar 24, 2021
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Mar 25, 2021

Great, thanks!

I am implementing time.Location("Americas/New_York") now and thinking about the best way to handle cases where the Location field has a double quote character or emoji or other character that needs escaping.

  • One solution would be to call strconv.AppendQuote which would add a dependency on strconv in the time package.

  • Another solution would be to copy the parts of AppendQuote that we need (we can simplify appendEscapedRune a little bit since we are only invoking it one way) into the time package and use them there.

  • Another one would be to copy the parts of AppendQuote that we need into internal/bytealg and then call it from there. Other packages in the standard library call strconv.Quote or strconv.AppendQuote, but all of them also call other functions in the strconv package so this wouldn't save us very much in terms of dependencies or compilation time.

  • Or some other solution involving omitting unprintable fields to avoid the need to take a dependency. This is an edge case of an edge case; most times are going to be UTC or Local and most zones in the tzdata database should contain characters that do not need escaping.

I'm not sure how strongly you feel about taking additional dependencies vs. copying code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants