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

remove usage of Newtonsoft.Json #2017

Merged
merged 10 commits into from Jan 20, 2020
Merged

remove usage of Newtonsoft.Json #2017

merged 10 commits into from Jan 20, 2020

Conversation

jetersen
Copy link
Contributor

@jetersen jetersen commented Jan 3, 2020

fixes #2009

name = $"GitVersion_{name}",
value = $"{value}"
};
var body = $"{{\"name\": \"GitVersion_{name}\",\"value\": \"{value}\"}}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt we need to evaluate whether value is an Integer or not.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure?

Copy link
Contributor Author

@jetersen jetersen Jan 17, 2020

Choose a reason for hiding this comment

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

Before it was going through JsonConvert.SerializeObject as object with the value of value = $"{value}" ergo it was turned into strings. The serialized JSON is unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Let's tackle that in a separate PR, if at all.

@jetersen
Copy link
Contributor Author

jetersen commented Jan 3, 2020

I see the appveyor actually tests the built DLL.

@jetersen
Copy link
Contributor Author

jetersen commented Jan 3, 2020

@asbjornu unsure why docker tests failed.

@arturcic
Copy link
Member

arturcic commented Jan 7, 2020

@Casz there are 2 more files to change, Directory.Build.props and test.props in the src folder

@arturcic arturcic added this to the 5.1.4 milestone Jan 7, 2020
@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

There are some tests that I rather not rewrite I think it's fine that tests use newtonsoft.json 😄

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

The problem that newtonsoft.json solves in tests is deserialize json to poco.

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

@arturcic I can use system.text.json instead for the tests if you want?

Or I can use JavaScriptSerializer to deserialize as it is not performance critical.

@arturcic
Copy link
Member

arturcic commented Jan 7, 2020

Sure works for me

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

Hmmm that's odd system.text.json is already included... Looking into which package pulls it in 🤔

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

Works for me:
image

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

I am not sure how this ever worked, it seems that Newtonsoft.Json was filtering out none JSON objects from a string object.

I dare not touch...

@jetersen
Copy link
Contributor Author

jetersen commented Jan 7, 2020

I see Newtonsoft.Json by default have no error handling setup.

So even if it receives an output string object like:

INFO [01/07/20 20:31:23:77] Applicable build agent found: 'TeamCity'.INFO [01/07/20 20:31:23:86] Working directory: C:\Users\josep\AppData\Local\Temp\TestRepositories\057e9db6-9c1a-4884-8ceb-89e8cc210785INFO [01/07/20 20:31:23:88] IsDynamicGitRepository: FalseINFO [01/07/20 20:31:23:92] Returning Project Root from DotGitDirectory: C:\Users\josep\AppData\Local\Temp\TestRepositories\057e9db6-9c1a-4884-8ceb-89e8cc210785\.git\ - C:\Users\josep\AppData\Local\Temp\TestRepositories\057e9db6-9c1a-4884-8ceb-89e8cc210785\INFO [01/07/20 20:31:23:92] Running on Windows.INFO [01/07/20 20:31:23:92] Applicable build agent found: 'TeamCity'.WARN [01/07/20 20:31:23:93] TeamCity doesn't make the current branch available through environmental variables.

Depending on your authentication and transport setup of your git VCS root things may work. In that case, ignore this warning.

In your TeamCity build configuration, add a parameter called `env.Git_Branch` with value %teamcity.build.vcs.branch.<vcsid>%

See http://gitversion.readthedocs.org/en/latest/build-server-support/build-server/teamcity for more infoINFO [01/07/20 20:31:23:93] Branch from build environment: INFO [01/07/20 20:31:23:93] Begin: Normalizing git directory for branch ''  INFO [01/07/20 20:31:23:98] One remote found (origin -> 'C:\git\code\GitVersion\src\GitVersionExe.Tests\bin\Debug\net472\TestRepositories\b58f239e-9bb0-41c2-9156-9160b6d64e8e').  INFO [01/07/20 20:31:23:98] Fetching from remote 'origin' using the following refspecs: +refs/pr/*/head:refs/remotes/origin/pr/*, +refs/heads/*:refs/remotes/origin/*.  INFO [01/07/20 20:31:24:01] Creating local branch from remote tracking 'refs/remotes/origin/FeatureBranch'.  INFO [01/07/20 20:31:24:04] End: Normalizing git directory for branch '' (Took: 110.00ms)  ERROR [01/07/20 20:31:24:18] An unexpected error occurred:
LibGit2Sharp.LibGit2SharpException: ref 'refs/remotes/origin/FeatureBranch' doesn't match the destination
   at LibGit2Sharp.Core.Ensure.HandleError(Int32 result)
   at LibGit2Sharp.Core.Proxy.git_refspec_rtransform(IntPtr refSpecPtr, String name)
   at LibGit2Sharp.Remote.FetchSpecTransformToSource(String reference)
   at LibGit2Sharp.BranchUpdater.GetUpstreamInformation(String canonicalName, String& remoteName, String& mergeBranchName)
   at LibGit2Sharp.BranchUpdater.SetUpstream(String upstreamBranchName)
   at GitVersion.Helpers.GitRepositoryHelper.<>c__DisplayClass8_1.<CreateOrUpdateLocalBranchesFromRemoteTrackingOnes>b__1(BranchUpdater b) in C:\git\code\GitVersion\src\GitVersionCore\Helpers\GitRepositoryHelper.cs:line 298
   at LibGit2Sharp.BranchCollection.Update(Branch branch, Action`1[] actions)
   at GitVersion.Helpers.GitRepositoryHelper.CreateOrUpdateLocalBranchesFromRemoteTrackingOnes(ILog log, Repository repo, String remoteName) in C:\git\code\GitVersion\src\GitVersionCore\Helpers\GitRepositoryHelper.cs:line 298
   at GitVersion.Helpers.GitRepositoryHelper.NormalizeGitDirectory(ILog log, IEnvironment environment, String gitDirectory, AuthenticationInfo authentication, Boolean noFetch, String currentBranch, Boolean isDynamicRepository) in C:\git\code\GitVersion\src\GitVersionCore\Helpers\GitRepositoryHelper.cs:line 41
   at GitVersion.GitPreparer.NormalizeGitDirectory(AuthenticationInfo auth, String targetBranch, String gitDirectory, Boolean isDynamicRepository) in C:\git\code\GitVersion\src\GitVersionCore\GitPreparer.cs:line 190
   at GitVersion.GitPreparer.Prepare(Boolean normalizeGitDirectory, String currentBranch, Boolean shouldCleanUpRemotes) in C:\git\code\GitVersion\src\GitVersionCore\GitPreparer.cs:line 62
   at GitVersion.GitVersionCalculator.CalculateVersionVariables() in C:\git\code\GitVersion\src\GitVersionCore\GitVersionCalculator.cs:line 52
   at GitVersion.ExecCommand.Execute() in C:\git\code\GitVersion\src\GitVersionExe\ExecCommand.cs:line 40
   at GitVersion.GitVersionExecutor.VerifyArgumentsAndRun(Arguments arguments) in C:\git\code\GitVersion\src\GitVersionExe\GitVersionExecutor.cs:line 117  INFO [01/07/20 20:31:24:18]   INFO [01/07/20 20:31:24:18] Attempting to show the current git graph (please include in issue):   INFO [01/07/20 20:31:24:18] Showing max of 100 commits  INFO [01/07/20 20:31:24:29] * 9bb62fb 52 minutes ago  (HEAD)
* 867dace 54 minutes ago  (origin/FeatureBranch, FeatureBranch)
* 9176cf2 56 minutes ago 
* b44da0d 58 minutes ago  (tag: 1.0.3, origin/master)

Such errors are completely ignored by Newtonsoft.Json.

@asbjornu
Copy link
Member

asbjornu commented Jan 8, 2020

How can such errors be ignored, @Casz? Newtonsoft.Json will throw regular Exceptions in the case of unparseable JSON, no?

@jetersen
Copy link
Contributor Author

Still think this is mergable, I can cleanup the Newtonsoft.Json in tests in a follow up PR.

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.

I think this looks good besides treating every value as string.

name = $"GitVersion_{name}",
value = $"{value}"
};
var body = $"{{\"name\": \"GitVersion_{name}\",\"value\": \"{value}\"}}";
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure?

@asbjornu
Copy link
Member

Hm. Is the serialization of integers related to #1688 in any way?

@asbjornu asbjornu merged commit d84c116 into GitTools:master Jan 20, 2020
@asbjornu
Copy link
Member

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

@jetersen
Copy link
Contributor Author

jetersen commented Jan 20, 2020

@asbjornu Thank you for maintaining the project 🙇

I believe the JSON Output formatter does not check for null or empty.
I have #2019 I can fix it there, once I get back to it.

@asbjornu
Copy link
Member

Sounds good, @Casz.

@jetersen jetersen mentioned this pull request Jan 23, 2020
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.

GitVersionTask 5.1.3 fails on appveyor FileLoadException
3 participants