8000 Installation: Use *Timestamp instead of *time.Time for the benefit of InstallationEvent by bmoylan · Pull Request #965 · google/go-github · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Installation: Use *Timestamp instead of *time.Time for the benefit of InstallationEvent #965

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

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

bmoylan
Copy link
Contributor
@bmoylan bmoylan commented Aug 5, 2018

According to https://developer.github.com/v3/activity/events/types/#installationevent the created_at and updated_at timestamps in the event payloads are unix-formatted, while the CRUD APIs use RFC3339. Using *Timestamp handles the unfortunate fact that the API is inconsistent. Without this change, my application emits the following error

failed to unmarshal PullRequestEvent: parsing time "1533435666" as ""2006-01-02T15:04:05Z07:00"": cannot parse "1533435666" as """

I understand this might be a breaking change to the public API, so I'm open to feedback if there are alternative ways to proceed.


This change is Reviewable

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Aug 5, 2018
@bmoylan
Copy link
Contributor Author
bmoylan commented Aug 5, 2018

@nmiyake

Copy link
Collaborator
@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @bmoylan!

You definitely found a bug, so we have no problems breaking the API when the current implementation doesn't work. We will need to label this repo with a new version for the breaking change.

LGTM.
Awaiting second LGTM before merging.

@gmlewis gmlewis requested a review from juliaferraioli August 5, 2018 12:29
Copy link
Contributor
@juliaferraioli juliaferraioli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice catch, @bmoylan!

LGTM.

@gmlewis
Copy link
Collaborator
gmlewis commented Aug 6, 2018

Thank you, @bmoylan and @juliaferraioli!
Merging.

@gmlewis gmlewis merged commit 2f5e75e into google:master Aug 6, 2018
@bmoylan bmoylan deleted the bm/installation-timestamp branch August 6, 2018 17:51
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018
PatWie added a commit to PatWie/pylint that referenced this pull request Aug 14, 2018
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0