deterministically serialize number and string JSON output #2527
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dieterv
force-pushed
the
feature/jsonserializer
branch
from
65d88a7
to
5f89021
Compare
asbjornu
requested changes
Jan 11, 2021
dieterv
force-pushed
the
feature/jsonserializer
branch
3 times, most recently
from
883d4af
to
bc0f79c
Compare
asbjornu
approved these changes
Feb 2, 2021
There was a problem hiding this comment.
I think the change from "" to null is an acceptable compromise and not enough of a breaking change to warrant a major version bump. I therefore give this 👍🏼. Great work! Please just rebase your changes on HEAD of main so we get rid of the merge conflicts and this is good to go.
Ever since the introduction of the JsonOutputFormatter way back in 2014 [1] all fields where serialized as JSON string. Except for values looking like an int which where then serialized as JSON numbers. This had some strange unpredictable side effects like PreReleaseNumber being an empty JSON string instead of null or a "2009069" ShortSha being formatted as a JSON number. This change ensures all fields are serialized as JSON string except Major, Minor, Patch, PreReleaseNumber, WeightedPreReleaseNumber, CommitsSinceVersionSource and UncommittedChanges which are now always serialized as JSON number. Deserialisation remains the same as before for all fields so we continue to accept both JSON string and JSON number formatted values. Fixes GitTools#1688 and fixes GitTools#2304. [1] GitTools@f2daf60#diff-fde1a8ff593c6ac2ad558a6f9bb512e0350db91343b185f9b2a00d1d6e848bc3
dieterv
force-pushed
the
feature/jsonserializer
branch
from
bc0f79c
to
d1baac1
Compare
|
@asbjornu wrote:
Thanks you for taking the time to review this! Rebased as requested 😃 |
arturcic
approved these changes
Feb 5, 2021
|
@dieterv thank you so much for your contributions 👍 |
|
Thank you @arturcic 😀 |
|
🎉 This issue has been resolved in version 5.6.5 🎉 Your GitReleaseManager bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Deterministically serialize number and string JSON output.
Related Issue
Fixes #1688 and fixes #2304.
Motivation and Context
Ever since the introduction of the JsonOutputFormatter way back
in 2014 [1] all fields where serialized as JSON string. Except
for values looking like an int which where then serialized as JSON
numbers. This had some strange unpredictable side effects like
PreReleaseNumber being an empty JSON string instead of null
or a "2009069" ShortSha being formatted as a JSON number.
This change ensures all fields are serialized as JSON string
except Major, Minor, Patch, PreReleaseNumber,
WeightedPreReleaseNumber, CommitsSinceVersionSource and
UncommittedChanges which are now always serialized as JSON number.
Deserialisation remains the same as before for all fields so we
continue to accept both JSON string and JSON number formatted
values.
[1] f2daf60#diff-fde1a8ff593c6ac2ad558a6f9bb512e0350db91343b185f9b2a00d1d6e848bc3
How Has This Been Tested?
Checklist: