add JSON Serializer #2060

jetersen · 2020-01-21T12:33:04Z

The problem with adding a JSON Serializer dependency is that you may have dependency conflicts with those using the same JSON serializer dependency.

Reason for #2017 😉 When using AppVeyor, that needed newtonsoft.json dep and the project uses newtonsoft.json in a different version will cause such a conflict.

jetersen · 2020-01-21T12:37:59Z

@gep13 new build failure 🤔

Time Elapsed 00:00:12.72
	TargetSite:	BuildVersion Calculate(Cake.Core.ICakeContext, BuildParameters, Cake.Common.Tools.GitVersion.GitVersion)
	StackTrace:	   at Submission#0.BuildVersion.Calculate(ICakeContext context, BuildParameters parameters, GitVersion gitVersion)
   at Submission#0.BuildParameters.Initialize(ICakeContext context, GitVersion gitVersion)
   at Submission#0.<<Initialize>>b__0_83(ISetupContext context)
	Message:	Object reference not set to an instance of an object.
	Data:	[  ]
	InnerException:	
	HelpLink:	
	Source:	ℛ*c0849450-dbab-4ef2-9cc0-640e37e6fbdb#1-0
	HResult:	-2147467261

However I am not seeing that locally

jetersen · 2020-01-21T12:56:28Z

Okay now I am just generating unexpected data:

[
  {
    "Key":"Major",
    "Value":5
  },
  {
    "Key":"Minor",
    "Value":1
  },
  {
    "Key":"Patch",
    "Value":4
  },
  {
    "Key":"PreReleaseTag",
    "Value":"json.1"
  },
  {
    "Key":"PreReleaseTagWithDash",
    "Value":"-json.1"
  },
  {
    "Key":"PreReleaseLabel",
    "Value":"json"
  },
  {
    "Key":"PreReleaseNumber",
    "Value":1
  },
  {
    "Key":"WeightedPreReleaseNumber",
    "Value":1
  },
  {
    "Key":"BuildMetaData",
    "Value":76
  },
  {
    "Key":"BuildMetaDataPadded",
    "Value":"0076"
  },
  {
    "Key":"FullBuildMetaData",
    "Value":"76.Branch.json.Sha.26623e2abbc134b7217d567d331b102b5793a1ed"
  },
  {
    "Key":"MajorMinorPatch",
    "Value":"5.1.4"
  },
  {
    "Key":"SemVer",
    "Value":"5.1.4-json.1"
  },
  {
    "Key":"LegacySemVer",
    "Value":"5.1.4-json1"
  },
  {
    "Key":"LegacySemVerPadded",
    "Value":"5.1.4-json0001"
  },
  {
    "Key":"AssemblySemVer",
    "Value":"5.1.4.0"
  },
  {
    "Key":"AssemblySemFileVer",
    "Value":"5.1.4.0"
  },
  {
    "Key":"FullSemVer",
    "Value":"5.1.4-json.1+76"
  },
  {
    "Key":"InformationalVersion",
    "Value":"5.1.4-json.1+76.Branch.json.Sha.26623e2abbc134b7217d567d331b102b5793a1ed"
  },
  {
    "Key":"BranchName",
    "Value":"json"
  },
  {
    "Key":"Sha",
    "Value":"26623e2abbc134b7217d567d331b102b5793a1ed"
  },
  {
    "Key":"ShortSha",
    "Value":"26623e2"
  },
  {
    "Key":"NuGetVersionV2",
    "Value":"5.1.4-json0001"
  },
  {
    "Key":"NuGetVersion",
    "Value":"5.1.4-json0001"
  },
  {
    "Key":"NuGetPreReleaseTagV2",
    "Value":"json0001"
  },
  {
    "Key":"NuGetPreReleaseTag",
    "Value":"json0001"
  },
  {
    "Key":"VersionSourceSha",
    "Value":"bef8ebc0b62b3ddd0cdafe09b66d68bbfcaf90d5"
  },
  {
    "Key":"CommitsSinceVersionSource",
    "Value":76
  },
  {
    "Key":"CommitsSinceVersionSourcePadded",
    "Value":"0076"
  },
  {
    "Key":"CommitDate",
    "Value":"2020-01-21"
  }
]

jetersen · 2020-01-21T13:08:42Z

I get it. This happens because we extends IEnumerable

jetersen · 2020-01-21T13:57:40Z

@asbjornu I am somewhat concerned about returning null's as it might change the behavior on too many places, it seems like a breaking change.

jetersen

Should we return null or keep empty strings "" in JSON?
Need to update tests either way.

jetersen · 2020-01-21T14:21:57Z

src/GitVersionCore/Helpers/JsonSerializer.cs

+{
+    // Credit to https://github.com/neuecc
+    // Inspired by https://gist.github.com/neuecc/7d728cd99d2a1e613362
+    public static class JsonSerializer


Is this overkill?

Hm. This turned out quite a bit more complicated than I had originally visioned, seeing how naïve the existing custom serialization code is. Is all of this necessary?

I can write a simpler one that only supports dictionary in one depth?

@asbjornu Please take another look, I chose a more simplified version that can only do objects.

Much simpler and sufficient, I think. If we need the full power of a full-fledged JSON serializer, we'll add a dependency to one, but I don't think that need is here at the moment.

The problem with adding such dependency is that you may have dependency conflicts with those using the same JSON serializer dependency.
Reason for #2017 😉 When using AppVeyor, that needed ewtonsoft.json dep and the project uses newtonsoft.json in a different version.

src/GitVersionCore/Helpers/JsonSerializer.cs

jetersen · 2020-01-21T14:23:40Z

src/GitVersionCore/BuildServers/AppVeyor.cs


-            var stringContent = new StringContent(body, Encoding.UTF8, "application/json");
+            var stringContent = new StringContent(JsonSerializer.Serialize(body), Encoding.UTF8, "application/json");


Hey, it's back using our own Serializer 🙇

jetersen · 2020-01-22T07:28:48Z

src/GitVersionCore/Extensions/ObjectExtensions.cs

+            value = kvp.Value;
+        }
+
+        public static IEnumerable<KeyValuePair<string, string>> GetProperties(this object obj)


Could properly use a better name 🤔

I think GetProperties is fine?

src/GitVersionCore/Extensions/ObjectExtensions.cs

asbjornu

I think this looks good! Tests are green and all is well. 😃

src/GitVersionCore/Extensions/ObjectExtensions.cs

asbjornu · 2020-01-22T22:15:01Z

src/GitVersionCore/Extensions/ObjectExtensions.cs

+            value = kvp.Value;
+        }
+
+        public static IEnumerable<KeyValuePair<string, string>> GetProperties(this object obj)


I think GetProperties is fine?

asbjornu · 2020-01-22T22:15:52Z

src/GitVersionCore/Helpers/JsonSerializer.cs

+{
+    // Credit to https://github.com/neuecc
+    // Inspired by https://gist.github.com/neuecc/7d728cd99d2a1e613362
+    public static class JsonSerializer


Much simpler and sufficient, I think. If we need the full power of a full-fledged JSON serializer, we'll add a dependency to one, but I don't think that need is here at the moment.

jetersen · 2020-01-24T14:32:28Z

all green @arturcic

arturcic · 2020-01-24T14:33:06Z

it's up to @asbjornu to merge, as he did the review

asbjornu · 2020-01-24T14:34:42Z

Thank you so much for your contributions, @Casz! 🙏

jetersen · 2020-01-24T14:37:10Z

Thank you for accepting my contributions @asbjornu 🙇

Just want to make it more clear why this was added, see the updated description.

add JSON Serializer

eb5145a

jetersen added 2 commits January 21, 2020 14:49

Respect ReflectionIgnore

508f405

Use our own JSON Serializer in AppVeyor

17bfb16

line breaks

8dacf4d

jetersen commented Jan 21, 2020

View reviewed changes

simplify

d42fe78

jetersen requested a review from asbjornu January 22, 2020 07:10

jetersen added 3 commits January 22, 2020 08:13

no bom, thanks

7de39cd

formatting

348b1e5

prevent null

8d64bfa

jetersen commented Jan 22, 2020

View reviewed changes

src/GitVersionCore/Extensions/ObjectExtensions.cs Outdated Show resolved Hide resolved

Merge branch 'master' into fix/remove-json-output-formatter

914f358

asbjornu approved these changes Jan 22, 2020

View reviewed changes

jetersen and others added 4 commits January 23, 2020 05:39

Mark ReflectionIgnoreAttribute as internal

22c2206

Merge branch 'master' into fix/remove-json-output-formatter

ed8a651

Merge branch 'master' into fix/remove-json-output-formatter

6304d7b

Merge branch 'master' into fix/remove-json-output-formatter

aad3ee8

asbjornu merged commit f0e5275 into GitTools:master Jan 24, 2020

arturcic added this to the 5.1.4 milestone Jan 24, 2020

arturcic added the feature label Jan 24, 2020

jetersen deleted the fix/remove-json-output-formatter branch January 24, 2020 14:38


		var stringContent = new StringContent(body, Encoding.UTF8, "application/json");
		var stringContent = new StringContent(JsonSerializer.Serialize(body), Encoding.UTF8, "application/json");

add JSON Serializer #2060

add JSON Serializer #2060

Uh oh!

Conversation

jetersen commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetersen commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetersen commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetersen commented Jan 21, 2020

Uh oh!

jetersen commented Jan 21, 2020

Uh oh!

jetersen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asbjornu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jetersen commented Jan 24, 2020

Uh oh!

arturcic commented Jan 24, 2020

Uh oh!

asbjornu commented Jan 24, 2020

Uh oh!

jetersen commented Jan 24, 2020

Uh oh!

Uh oh!

jetersen commented Jan 21, 2020 •

edited

Loading

jetersen commented Jan 21, 2020 •

edited

Loading

jetersen commented Jan 21, 2020 •

edited

Loading

jetersen left a comment •

edited

Loading