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

Migrate IDs to int64 type #816

Merged
merged 5 commits into from Jan 25, 2018

Conversation

Projects
None yet
7 participants
@kshitij10496
Collaborator

kshitij10496 commented Dec 23, 2017

Through this PR, I attempt to complete the migration of all the struct IDs(which refer to GitHub IDs) from platform dependent int type to the signed int64.

Also, a helper function Int64 is implemented to assist the migration of unit tests.

Checklist

  • Update IDs to int64
  • Updated unit tests
  • Updated Integration tests
  • Documentation

Reference: #597

@googlebot googlebot added the cla: yes label Dec 23, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 23, 2017

Member

The issue is with the go generate command making some unwanted changes to github/github-accessors.go.

I think the fix will involve changing case "int" into case "int", "int64" on this line.

Member

dmitshur commented Dec 23, 2017

The issue is with the go generate command making some unwanted changes to github/github-accessors.go.

I think the fix will involve changing case "int" into case "int", "int64" on this line.

@anubhakushwaha

This comment has been minimized.

Show comment
Hide comment
@anubhakushwaha

anubhakushwaha Dec 24, 2017

Contributor

@shurcooL @kshitij10496 I am facing the same travis fail error in #817 and I tried your suggestion link but it's still failing, any inputs?

Contributor

anubhakushwaha commented Dec 24, 2017

@shurcooL @kshitij10496 I am facing the same travis fail error in #817 and I tried your suggestion link but it's still failing, any inputs?

@anubhakushwaha

This comment has been minimized.

Show comment
Hide comment
@anubhakushwaha

anubhakushwaha Dec 24, 2017

Contributor

It got resolved by fixing the formatting as instructed by @kshitij10496 .

Contributor

anubhakushwaha commented Dec 24, 2017

It got resolved by fixing the formatting as instructed by @kshitij10496 .

@kshitij10496

This comment has been minimized.

Show comment
Hide comment
@kshitij10496

kshitij10496 Dec 24, 2017

Collaborator

@shurcooL I have made the necessary changes based on your suggestion.
Also, shouldn't we mention the fact that the client now uses int64 ID values in the documentation?

Collaborator

kshitij10496 commented Dec 24, 2017

@shurcooL I have made the necessary changes based on your suggestion.
Also, shouldn't we mention the fact that the client now uses int64 ID values in the documentation?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 5, 2018

Member

Also, shouldn't we mention the fact that the client now uses int64 ID values in the documentation?

Sure, if there's a good place for it. Can you think of one?

I don't think it's critical for getting this in. We can take care of it later if desired. In any case, it'll be something for reviewers to look out when doing future reviews (because some contributors can miss the documentation).

Member

dmitshur commented Jan 5, 2018

Also, shouldn't we mention the fact that the client now uses int64 ID values in the documentation?

Sure, if there's a good place for it. Can you think of one?

I don't think it's critical for getting this in. We can take care of it later if desired. In any case, it'll be something for reviewers to look out when doing future reviews (because some contributors can miss the documentation).

@dmitshur dmitshur requested a review from gmlewis Jan 5, 2018

@gmlewis

gmlewis approved these changes Jan 5, 2018

Gnarly!
(That's a "good thing" from the 80's 😄 )

@willnorris - I can't remember now if you also wanted to add a type ID int64 to the code or leave it like this?

It would cause quite a bit of churn for this one PR, but now is the time to do that if we decide we want it. I remember discussing the pros/cons and it seemed like it will cause the same amount of disturbance to end users of this repo with or without that change, if I recall... so I'm fine either way.

Thank you, @kshitij10496 and @shurcooL!

@gmlewis gmlewis requested a review from willnorris Jan 5, 2018

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Jan 5, 2018

Collaborator

Oh! Let's please not forget to bump libraryVersion in this PR, @kshitij10496, since this is a rather major change.

Collaborator

gmlewis commented Jan 5, 2018

Oh! Let's please not forget to bump libraryVersion in this PR, @kshitij10496, since this is a rather major change.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 5, 2018

Member

I can't remember now if you also wanted to add a type ID int64 to the code or leave it like this?

@gmlewis There was relevant discussion in the issue (#597), specifically this #597 (comment).

Member

dmitshur commented Jan 5, 2018

I can't remember now if you also wanted to add a type ID int64 to the code or leave it like this?

@gmlewis There was relevant discussion in the issue (#597), specifically this #597 (comment).

@kshitij10496

This comment has been minimized.

Show comment
Hide comment
@kshitij10496

kshitij10496 Jan 11, 2018

Collaborator

I've updated the libraryVersion in the github.go file.

Collaborator

kshitij10496 commented Jan 11, 2018

I've updated the libraryVersion in the github.go file.

@gmlewis gmlewis requested review from dmitshur and removed request for willnorris Jan 11, 2018

Show outdated Hide outdated github/event_types.go Outdated

@dmitshur dmitshur referenced this pull request Jan 16, 2018

Closed

int64 IDs #597

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 16, 2018

Member

This is a large PR. Thank you again @kshitij10496!

I've reviewed the fields that were modified first. They match up with the table in #597 (comment), with the exception of PullRequestEvent.Number that I asked about above, and 4 other ID fields that are valid but were missing in that table (I've added them now).

Then I reviewed the method parameters that were modified. This was harder to do, but the ones I saw modified look right. I can't be sure if any are missing though (but that's easier to fix in the future, if needed).

Overall, once we come to a resolution about PullRequestEvent.Number, I think this looks good. At least I'm not seeing anything wrong.

It's still a bit scary because this is going to be a relatively large breaking change.

@bradfitz Can you look this over and confirm if this PR looks as you'd expect it to for resolving #597?

Member

dmitshur commented Jan 16, 2018

This is a large PR. Thank you again @kshitij10496!

I've reviewed the fields that were modified first. They match up with the table in #597 (comment), with the exception of PullRequestEvent.Number that I asked about above, and 4 other ID fields that are valid but were missing in that table (I've added them now).

Then I reviewed the method parameters that were modified. This was harder to do, but the ones I saw modified look right. I can't be sure if any are missing though (but that's easier to fix in the future, if needed).

Overall, once we come to a resolution about PullRequestEvent.Number, I think this looks good. At least I'm not seeing anything wrong.

It's still a bit scary because this is going to be a relatively large breaking change.

@bradfitz Can you look this over and confirm if this PR looks as you'd expect it to for resolving #597?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 18, 2018

Contributor

I'd prefer if fixes within this PR didn't accumulate in new commits (like 4c02471).

I'd just rebase/squash mistakes and have commits that stand alone. It'll be make git archaeology in the future easier.

Contributor

bradfitz commented Jan 18, 2018

I'd prefer if fixes within this PR didn't accumulate in new commits (like 4c02471).

I'd just rebase/squash mistakes and have commits that stand alone. It'll be make git archaeology in the future easier.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 18, 2018

Member

@bradfitz We (almost always) merge PRs via squash strategy, so the entire diff will be just one commit once it lands to master (e.g., see https://github.com/google/go-github/pull/812/commits, which was 6 commits that got merged as a single commit 218ecc2).

We encourage contributors not to squash during the course of the PR, because that makes it harder to review and see what has changed since last review (this isn't Gerrit; you can't see old versions of a PR).

Does that address your concern?

Member

dmitshur commented Jan 18, 2018

@bradfitz We (almost always) merge PRs via squash strategy, so the entire diff will be just one commit once it lands to master (e.g., see https://github.com/google/go-github/pull/812/commits, which was 6 commits that got merged as a single commit 218ecc2).

We encourage contributors not to squash during the course of the PR, because that makes it harder to review and see what has changed since last review (this isn't Gerrit; you can't see old versions of a PR).

Does that address your concern?

Show outdated Hide outdated github/event_types.go Outdated

kshitij10496 added some commits Dec 23, 2017

Migrate IDs to int64 type
In this commit, the complete migration of all the struct IDs(which refer
to GitHub IDs) from platform dependent `int` type to the signed `int64`
takes place.

A helper function "Int64" is also implemented to assist the migration of
unit tests.

Refer #597
@kshitij10496

This comment has been minimized.

Show comment
Hide comment
@kshitij10496

kshitij10496 Jan 18, 2018

Collaborator

@shurcooL I have updated the PR after generating the code. Thanks for the suggestion.
@bradfitz If you have any other suggestions related to this PR, I would be more than happy to work on it. 😄

Collaborator

kshitij10496 commented Jan 18, 2018

@shurcooL I have updated the PR after generating the code. Thanks for the suggestion.
@bradfitz If you have any other suggestions related to this PR, I would be more than happy to work on it. 😄

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 19, 2018

Contributor

We (almost always) merge PRs via squash strategy

Ah, great. You can tell I don't use GitHub PRs much, huh? :)

Contributor

bradfitz commented Jan 19, 2018

We (almost always) merge PRs via squash strategy

Ah, great. You can tell I don't use GitHub PRs much, huh? :)

@kshitij10496

This comment has been minimized.

Show comment
Hide comment
@kshitij10496

kshitij10496 Jan 20, 2018

Collaborator

@shurcooL Is there anything else that we need to address in this PR related to the issue?

Collaborator

kshitij10496 commented Jan 20, 2018

@shurcooL Is there anything else that we need to address in this PR related to the issue?

@dmitshur

There isn’t anything that I’m aware of. I think all that’s left is for us to take a deep breath and pull the trigger.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 20, 2018

Member

One extra thing I can do to increase confidence before merging is apply this PR locally and see how it looks on all my projects that import go-github. There’s a chance it’ll help catch something we can’t see just by looking at the diff.

Member

dmitshur commented Jan 20, 2018

One extra thing I can do to increase confidence before merging is apply this PR locally and see how it looks on all my projects that import go-github. There’s a chance it’ll help catch something we can’t see just by looking at the diff.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 20, 2018

Member

Checked it out locally, it went well. Out of the 7 non-toy packages I have that directly import go-github, I only needed to update one type conversion in one package. I really expected there to be more breakages (but of course other packages may vary).

It's probably nice to wait until Monday (so people who don't vendor their dependencies can enjoy their weekend without breaking API changes), but otherwise we can merge this anytime if there are no further objections.

Member

dmitshur commented Jan 20, 2018

Checked it out locally, it went well. Out of the 7 non-toy packages I have that directly import go-github, I only needed to update one type conversion in one package. I really expected there to be more breakages (but of course other packages may vary).

It's probably nice to wait until Monday (so people who don't vendor their dependencies can enjoy their weekend without breaking API changes), but otherwise we can merge this anytime if there are no further objections.

@sahildua2305

@kshitij10496 really nice work here! I have been a quiet reviewer/observer on this PR and really liked the conversations/suggestions brought up. Keep it up. 🙂

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 25, 2018

Member

Ping @gmlewis. Shall we proceed?

Member

dmitshur commented Jan 25, 2018

Ping @gmlewis. Shall we proceed?

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Jan 25, 2018

Collaborator

Yes, LGTM.
Merging.

Collaborator

gmlewis commented Jan 25, 2018

Yes, LGTM.
Merging.

@gmlewis gmlewis merged commit e48060a into google:master Jan 25, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kshitij10496

This comment has been minimized.

Show comment
Hide comment
@kshitij10496

kshitij10496 Jan 25, 2018

Collaborator

I want to thank everyone involved in the discussion on #597 which immensely helped me with the implementation.

Collaborator

kshitij10496 commented Jan 25, 2018

I want to thank everyone involved in the discussion on #597 which immensely helped me with the implementation.

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Jan 25, 2018

Collaborator

Thank you for the PR, @kshitij10496!

Collaborator

gmlewis commented Jan 25, 2018

Thank you for the PR, @kshitij10496!

dghubble added a commit to dghubble/gologin that referenced this pull request Jan 26, 2018

Fix github tests to use int64 user ids
* Package github.com/google/go-github has migrated
type User ids from type int to int64
* google/go-github#816

gopherbot pushed a commit to golang/build that referenced this pull request Feb 19, 2018

maintner: add GerritCL.CommitAtVersion method
Previous versions were available via GerritProject.remote, but that
symbol is not exported and can't be used by callers of this package.

Update test for go-github API change in google/go-github#816, so the
tests can pass locally. Since go-github isn't vendored, it's understood
that the latest upstream version is targeted.

Change-Id: Ib4b78abacb19d73f95df2215b4a248a24376ae10
Reviewed-on: https://go-review.googlesource.com/95136
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Migrate IDs to int64 type (google#816)
A helper function "Int64" is also implemented to assist the migration of
unit tests.

Refer google#597.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment