Skip to content

Change "IssueId" to long#2890

Merged
kfcampbell merged 3 commits intooctokit:mainfrom
Leh2:2889-change-issue-id-to-long
Feb 23, 2024
Merged

Change "IssueId" to long#2890
kfcampbell merged 3 commits intooctokit:mainfrom
Leh2:2889-change-issue-id-to-long

Conversation

@Leh2
Copy link
Contributor

@Leh2 Leh2 commented Feb 22, 2024

Resolves #2889


Before the change?

  • New issues couldn't be handled by the client because GitHub IDs have surpassed int.Max.

After the change?

  • New issues know works in the client

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Consumers using the "Id" will experience a breaking change and need to update their type to long.

Please see our docs on breaking changes to help!

  • Yes
  • No

/// The internal Id for this issue (not the issue number)
/// </summary>
public int Id { get; private set; }
public long Id { get; private set; }
Copy link
Contributor

@arxange1 arxange1 Feb 22, 2024

Choose a reason for hiding this comment

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

int id in the constructor to be fixed as well.

There is also NewPullRequest class with the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - fixed in b1d53cc and cc2dd92

@Leh2 Leh2 requested a review from arxange1 February 22, 2024 10:45
@JimSuplizio
Copy link

Crazy question, related to the PR after it gets merged. What's the timeline between this getting merged and a release being up on nuget?

Copy link

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

Unfortunately, there's a lot more that needs to be done than just changing the Id in Issue.cs. For example, the IIssuesClient.cs uses int all over the place for the Issue number (Id). Further, does the GitHub API even support long when you're updating an issue? As far as I can tell, the Latest Rest API version, which is what Octokit is using, only documents Int, not long.

@Leh2
Copy link
Contributor Author

Leh2 commented Feb 22, 2024

Unfortunately, there's a lot more that needs to be done than just changing the Id in Issue.cs. For example, the IIssuesClient.cs uses int all over the place for the Issue number (Id). Further, does the GitHub API even support long when you're updating an issue? As far as I can tell, the Latest Rest API version, which is what Octokit is using, only documents Int, not long.

Issue number is not the same as issue id.

@JimSuplizio
Copy link

Issue number is not the same as issue id.

You are correct, I am in error.

Copy link

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

My original review was incorrect. @Leh2 has enlightened me. This fix should be good.

@danmoseley
Copy link

are there any other values that will roll over in the near future and should change to long now as well?

@kfcampbell
Copy link
Contributor

Thank you for the quick fix! Please report back if something seems amiss.

@kfcampbell kfcampbell merged commit d27d1f0 into octokit:main Feb 23, 2024
@Leh2
Copy link
Contributor Author

Leh2 commented Feb 23, 2024

Thank you for the quick fix! Please report back if something seems amiss.

Just tested the new package, and everything works as expected again 👍 Thank you for the NuGet release 💪

Mankarse added a commit to Mankarse/Zero-K-Infrastructure that referenced this pull request Mar 31, 2024
Old Octokit versions used int rather than long for Issue.Id;
which causes overflow error when parsing responses from the GitHub Issues API.
See octokit/octokit.net#2890
Licho1 pushed a commit to ZeroK-RTS/Zero-K-Infrastructure that referenced this pull request Apr 6, 2024
Old Octokit versions used int rather than long for Issue.Id;
which causes overflow error when parsing responses from the GitHub Issues API.
See octokit/octokit.net#2890

(cherry picked from commit 80c8169)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG]: Error deserializing new issues

5 participants