Change all fields from time.Time to github.Timestamp

[zombieleet]

This PR introduces breaking API changes as described here #2645
It also fix the related test for the breaking API changes

[codecov]

Codecov Report

Merging #2646 (d65736f) into master (36c47d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2646   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files         130      130           
  Lines       11242    11244    +2     
=======================================
+ Hits        11023    11025    +2     
  Misses        150      150           
  Partials       69       69           

Impacted Files	Coverage Δ
github/apps.go	95.16% <ø> (ø)
github/event.go	100.00% <ø> (ø)
github/gists.go	96.68% <ø> (ø)
github/gists_comments.go	100.00% <ø> (ø)
github/git_commits.go	100.00% <ø> (ø)
github/issues.go	97.41% <ø> (ø)
github/issues_comments.go	100.00% <ø> (ø)
github/issues_events.go	100.00% <ø> (ø)
github/issues_milestones.go	100.00% <ø> (ø)
github/issues_timeline.go	100.00% <ø> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[zombieleet]

the examples project was having a type error, it was fixed by using github.Timestamp, which caused another error in go vet relating to unkeyed fields. excluding composites fixes the error

[gmlewis]

Curious what we were missing by excluding composites, I ran across these issues:

• go vet: allow disabling composites analysis golangci/golangci-lint#446
• proposal: cmd/vet: relax "composites" check to module scope golang/go#43864

in case anyone in the future finds this and wonders about it.

[gmlewis]

@willnorris - this is a significant breaking change affecting a large number of fields.

Personally, I'm fine with it, but before we make such a sweeping change, I'd like to make sure you are fine with this too.

[willnorris]

In general, I don't really love moving away from standard Go types where it's not actually necessary. It makes it harder to interop with other libraries and existing code. In hindsight, I kinda wonder if we could have made Timestamp a time.Time rather than a struct containing a Time. (type Timestamp time.Time) But whatever, that ship has long sailed.

It looks like github.Timestamp was introduced in c6d364f for timestamps in the repo struct. It looks like we're already using github.Timestamp quite a bit in the library, though I'm curious how much of that is actually needing to parse unix timestamps versus just doing what other parts of the library are doing. At the end of the day, I guess it probably doesn't matter too much.

Given that timestamps are almost exclusively read (I don't know of any GitHub API off the top of my head that involves providing a timestamp), and github.Timestamp includes all of the same methods as time.Time, I think this is probably fine. I haven't reviewed all of the changes in this PR, but the general approach sounds okay.

[gmlewis]

Thank you, @willnorris and @zombieleet !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

[ganeshkumarsv]

LGTM!

[gmlewis]

Thank you, @ganeshkumarsv !
Merging.

[gmlewis]

Fixes: #2645.