Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change all applicable IDs from int to long #30

Closed
JamieMagee opened this issue Jan 16, 2022 · 4 comments · Fixed by #38
Closed

Change all applicable IDs from int to long #30

JamieMagee opened this issue Jan 16, 2022 · 4 comments · Fixed by #38

Comments

@JamieMagee
Copy link
Contributor

See:
@nwf
Copy link

nwf commented Jan 17, 2022

I suspect you've raised this now for the same reason I have suddenly found it important, but just to document, for posterity if nothing else: webhook_job ids are expressed in the JSON as numbers, and they're well and truly above 2**32. :(

@moodymatt
Copy link

Anytime an issue webhook event has a label, the id of the label is exceeds the size of an int and the json deserialization fails.

Obviously this applies to more than just the Label.Id but I'm adding a comment to get some additional attention on this. It's an absolute deal breaker for this SDK.

JamieMagee added a commit that referenced this issue Jan 24, 2022
@JamieMagee
fix: migrate from int to long
e227e7b 
Migrates all integer types from `int` (32-bit) to `long` (64-bit).

Could I be a bit more directed with these changes? Probably. But the it's simpler and easier to migrate all `int`s to `long`s rather than debate how large a specific ID is going to be.

Closes #30

References:
  - #30
  - octokit/octokit.net#2351
  - octokit/octokit.net#2352
  - https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/integral-numeric-types
@JamieMagee JamieMagee mentioned this issue Jan 24, 2022
Merged
@JamieMagee
Copy link
Contributor Author

@moodymatt you're right. I haven't had much time recently to dedicate to maintenance here, but I opened a quick PR at #38.

@moodymatt
Copy link

@moodymatt you're right. I haven't had much time recently to dedicate to maintenance here, but I opened a quick PR at #38.

@JamieMagee awesome work and thanks for the quick response! I'll keep an eye out for the PR completing and update my references to the latest version of the SDK once all the checks have passed and its been merged down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: migrate from int to long octokit/webhooks.net
3 participants
@nwf @JamieMagee @moodymatt