-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Test-Json: Use JsonSchema.Net (System.Text.Json) instead of NJsonSchema (Newtonsoft.Json) #18141
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
Test-Json: Use JsonSchema.Net (System.Text.Json) instead of NJsonSchema (Newtonsoft.Json) #18141
Conversation
src/Microsoft.PowerShell.Commands.Utility/commands/utility/TestJsonCommand.cs
Outdated
Show resolved
Hide resolved
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.
One change requested to put error string in resx otherwise LGTM
6aba9c3
to
df01974
Compare
I'm not sure what the issue is with the various builds. I can't run the macos one, and I've never been able to get the files.wxs file regenerated right (it breaks every time I have to rebase). As an aside, why is this file even included in the repo if it's generated at build time anyway? |
@gregsdennis I can not reproduce the issue locally and I hope anybody from MSFT team will help to resolve the CI issue. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@SteveL-MSFT I could use some help getting the wxs file fixed. It has never generated right for me. This happens every time I have to rebase. |
@SteveL-MSFT I cannot keep updating this branch because I can't generate the .wxs file. Please assist. I'm not sure why this PR has languished this long. It seems like a straightforward change. |
Can anyone help with this? Is there any reason why it hasn't been pulled in? |
@SteveL-MSFT tagging again for an update. |
@gregsdennis sorry for the delay, I'm re-reviewing it today |
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.
LGTM. @gregsdennis Thanks for your contribution, and I really appreciate your patience!
@gregsdennis Please look test fails. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@SteveL-MSFT @daxian-dbw Please update your review (large rebase was). |
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.
New changes after my last review looks good to me.
@gregsdennis Thanks for your contribution! |
🎉 Handy links: |
PR Summary
Updates the
Test-Json
cmdlet to use JsonSchema.Net instead of NJsonSchema in order to:(continuation of #18023 - see here for additional comments)
PR Context
Resolves #18009
NJsonSchema only supports up to draft 7, which at this point is several years old and two versions behind. Additionally, it relies on Newtonsoft.Json.
This change is a merely drop-in replacement of the implementation that provides validation.
The cmdlet API doesn't change, however the cmdlet will no longer support draft 4 schemas. Draft 4 is a decade old at this point and the JSON Schema team (myself included) is encouraging tooling to only support draft 6/7 (6 is a subset of 7) and later. (In the future, support for these should be dropped as newer versions are released.) I'm not sure if this is considered a breaking change for this repo.
To assist users in their migration away from draft 4, the alterschema draft migration tool has been created.
I expect the existing test coverage is adequate to cover this change.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).