-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Migrate Test-Json to System.Text.Json API #11397
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
Conversation
f63a0ee to
4280157
Compare
|
@mklement0 @jochenvanwylick @thedavecarroll @vexx32 The PR was moved to .Net Runtime 5.0 Preview1. Please play with compiled artifacts and feedback. |
4280157 to
142aa60
Compare
src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
Outdated
Show resolved
Hide resolved
142aa60 to
daf3271
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these values be unquoted? Doesn't JSON accept true/false/null as primitive values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you thing about ConvertTo-Json. Test-Json accepts string input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but that's already tested in the case below. Is this not intended to verify that it accepts the true/false/null literals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see your point.
Add new tests
5fe5849 to
8e403e7
Compare
8e403e7 to
cfdb351
Compare
src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
Outdated
Show resolved
Hide resolved
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
cfdb351 to
d9d8936
Compare
|
Rebase to resolve merge conflicts. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
d9d8936 to
7122071
Compare
7122071 to
54e73bf
Compare
54e73bf to
1adb626
Compare
|
Rebased to move 5.0 RC2. |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
Outdated
Show resolved
Hide resolved
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
Doesn't this change actually cause a performance drop as it causes the validated JSON to be parsed twice in |
PR Summary
Before the change we used Newtonsoft Json.NET
JObject.Parse()method to validate Json. The method is not best choice to validate Json because it doesn't parse valid primitives.As result we:
Formally it is a breaking change because Newtonsoft Json.NET
JObject.Parse()accepted single quote delimiters.PR Context
Fix #11384
Need review #5797
.Net docs https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.