Skip to content

Upgrade RepoStatus.ID from int to int64 #807

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

Closed
wants to merge 2 commits into from
Closed

Upgrade RepoStatus.ID from int to int64 #807

wants to merge 2 commits into from

Conversation

maruel
Copy link

@maruel maruel commented Dec 10, 2017

This is needed on 32 bits platforms. Right now it is impossible to call
CreateStatus() on 32 bits executable because the ID returned by Github
overflows.

This is an API breaking change.

Fixes #806

This is needed on 32 bits platforms. Right now it is impossible to call
CreateStatus() on 32 bits executable because the ID returned by Github
overflows.

This is an API breaking change.

Fixes #806
@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Dec 10, 2017
@maruel
Copy link
Author

maruel commented Dec 10, 2017

I do not address the other IDs as I confirmed this change is sufficient to get gohci back into working state and this reduces the amount of API breakage.

It seems the generated file was stale prior to my change. I'm not sure why
GetID() exists, it's not useful.
@sahildua2305
Copy link
Member

I'm not sure why GetID() exists, it's not useful.

@maruel answer to your comment in commit 8428089 - Since RepoStatus is an exported struct, it will have automatically generated accessors for all of its exported fields, unless the struct itself is blacklisted or some of its fields are blacklisted.

@dmitshur
Copy link
Member

We should not merge this PR until we make a decision in #597. Merging it sooner would make a decision implicitly, cause an API breakage, and put the library in an inconsistent state (ints in some places, int64 in others), and make resolving this issue even harder.

This library has a lot of users, and the vast majority of them are on 64-bit architectures, so they're not negatively affected (their ints are 64-bit). For those on 32-bit systems that need this, they should apply the change ad-hoc until we can resolve the issue upstream in a way that's viable, which is what @maruel said he has already done in #806 (comment).

Thanks for sending this PR @maruel, but I'll close it for now to avoid this PR being merged by accident (it can easily be re-opened when needed). We should continue further discussion and come up with a plan of action in #597.

@dmitshur dmitshur closed this Dec 14, 2017
if r == nil || r.ID == nil {
return 0
// GetID returns the ID field.
func (r *RepoStatus) GetID() *int64 {
Copy link
Member

Choose a reason for hiding this comment

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

For posterity (no need to resolve this now), the generator is not doing the right thing for int64 type. It should be generating the following signature:

func (r *RepoStatus) GetID() int64

But it's treating int64 as if it were a struct.

@dmitshur dmitshur mentioned this pull request Dec 15, 2017
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