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.

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

Handle int64 numbers when writing version variables #3248

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

RatserX
Copy link
Contributor

@RatserX RatserX commented Oct 27, 2022

Description

Long numeric values for certain version variable fields (e.g. PreReleaseNumber) cause the number parsing to fail and throw the InvalidOperationException.
WriteNumberValue supports int64 so this should be a pretty safe update.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@asbjornu
Copy link
Member

asbjornu commented Nov 2, 2022

This seems like something we should release in v5. It also feels like something that should be roll into @james-bjss' #2703 which also deals with integer parsing. Perhaps you can pick up his PR and rebase it against support/5.x, @RatserX?

@RatserX RatserX changed the base branch from main to support/5.x November 3, 2022 01:41
@RatserX
Copy link
Contributor Author

RatserX commented Nov 3, 2022

This seems like something we should release in v5. It also feels like something that should be roll into @james-bjss' #2703 which also deals with integer parsing. Perhaps you can pick up his PR and rebase it against support/5.x, @RatserX?

I just rebased against support/5.x. I checked that PR but it seems that we're already handling int64 numbers, so we shouldn't run into overflow exceptions in the scenario that PR was fixing. Do we still need to do a TryParse?

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

True, #3028 did most of that but you've found a missed or new occurrence, which is great. I still think there's value in consistently having a TryParse() pattern, but as it's going to fail a lot less with long than int, I think we can merge this and leave #2703 open as a avenue for future improvement.

@asbjornu asbjornu merged commit 9969dab into GitTools:support/5.x Nov 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 3, 2022

Thank you @RatserX for your contribution!

@asbjornu asbjornu added this to the 5.x milestone Nov 3, 2022
@RatserX
Copy link
Contributor Author

RatserX commented Nov 3, 2022

@asbjornu Is there any ETA as to when a new 5.x version is going to be released? This change would actually help us unblock a lot of our projects.

@arturcic arturcic modified the milestones: 5.x, 5.11.0 Nov 7, 2022
@arturcic
Copy link
Member

arturcic commented Nov 7, 2022

🎉 This issue has been resolved in version 5.11.0 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants