Use Timestamp type for all time fields.

[dmitshur]

This is a breaking API change. It improves usability of time fields by automatically parsing them into a time.Time-like type.

Pointers are used for optional fields, so that the ,omitempty option correctly omits them when they have zero value (i.e., nil). This can't be done with values at this time (see golang/go#11939).

Resolves #55.

I used Gerrit REST API Documentation to find which fields are a timestamp. I hope I got them all.

[codecov-io]

Codecov Report

Merging #56 into master will increase coverage by 0.35%. The diff coverage is 62.5%.

@@            Coverage Diff            @@
##           master     #56      +/-   ##
=========================================
+ Coverage   22.34%   22.7%   +0.35%     
=========================================
  Files          21      21              
  Lines        1781    1797      +16     
=========================================
+ Hits          398     408      +10     
- Misses       1332    1335       +3     
- Partials       51      54       +3

Impacted Files	Coverage Δ
changes.go	16.14% <ø> (ø)	:arrow_up:
accounts.go	3.53% <ø> (ø)	:arrow_up:
groups.go	0% <ø> (ø)	:arrow_up:
projects_tag.go	0% <ø> (ø)	:arrow_up:
types.go	76.66% <62.5%> (-16.2%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5416038...588dd32. Read the comment docs.

[andygrunwald]

As mentioned in #55, i am with you to break the API here for convenience reasons :)

[...] I hope I got them all.

If not, we will adjust the missing ones later. I am in favor with a move fast, fail fast approach instead of delaying improvements for perfection.

[andygrunwald]

And of course: Thanks a lot. This are big steps in the right direction.

[dmitshur]

Thank you for the quick review and merge @andygrunwald!

In hindsight, I should've added a "DO NOT MERGE" note because I didn't want this to be merged right away; I just wanted to send out the PR for initial review because it was ready for review, but I also wanted to do a bit more before merging it.

[...] I hope I got them all.

If not, we will adjust the missing ones later. I am in favor with a move fast, fail fast approach instead of delaying improvements for perfection.

Yeah, I'm not against merging this sooner and updating any missed fields to use Timestamp type in the future, but I wanted us to use this PR to review the Timestamp API more thoroughly.

A good general practice in the Go project is to let the CL owner submit their own CLs (assuming they have the submit rights). That way, they can make any final tweaks to the commit message, any final testing, prepare any followup CLs, etc., before merging. If you don't mind, I'd prefer we do this here as well. Since I have the rights to merge PRs, you can just leave a LGTM review and I can merge when it's completely ready from my side.

This isn't a big deal in this case, but I want to ask for future, if it's okay with you.

Since this is a very recently merged change, we can still make any followup changes if needed.

[dmitshur]

The thing I wanted to do was ping @willnorris who worked in a very similar API in go-github and ask for his thoughts on the Timestamp type API as implemented here.

Since the go-gerrit library has fewer users and we're still able to make breaking API changes, it's a good opportunity to consider any additional improvements that perhaps @willnorris wanted to go into go-github's Timestamp type, but it's hard to make those changes now.

@willnorris, how does this Timestamp look to you?

One thing that is a bit different about it is that I've documented the underlying time.Time value that its time zone must be UTC:

// Timestamp represents an instant in time with nanosecond precision, in UTC time zone.
// It encodes to and from JSON in Gerrit's timestamp format.
// All exported methods of time.Time can be called on Timestamp.
//
// Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api.html#timestamp
type Timestamp struct {
    // Time is an instant in time. Its time zone must be UTC.
    time.Time
}

// MarshalJSON implements the json.Marshaler interface.
// The time is a quoted string in Gerrit's timestamp format.
// An error is returned if t.Time time zone is not UTC.
func (t Timestamp) MarshalJSON() ([]byte, error) {
    if t.Location() != time.UTC {
        return nil, errors.New("Timestamp.MarshalJSON: time zone must be UTC")
    }
    ...
}

The alternative could've been not to require that, and just convert Time to UTC inside MarshalJSON.

However, I wanted to try going with the more strict approach of requiring UTC because it felt like a better mapping to the Gerrit timestamp type, which always uses UTC.

[andygrunwald]

In hindsight, I should've added a "DO NOT MERGE" note because I didn't want this to be merged right away; I just wanted to send out the PR for initial review because it was ready for review, but I also wanted to do a bit more before merging it.

Oh okay. Feel free to revert it in master.

A good general practice in the Go project is to let the CL owner submit their own CLs (assuming they have the submit rights) [...] If you don't mind, I'd prefer we do this here as well. Since I have the rights to merge PRs, you can just leave a LGTM review and I can merge when it's completely ready from my side.

Nice approach. I like it. Lets try it :)