Skip to content

Add IgnoreComments and AllowTrailingCommas options to Test-Json cmdlet #23817

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.

Sign up for GitHub

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

Jump to bottom
Merged
iSazonov merged 4 commits into from
Jun 24, 2024
Merged

Add IgnoreComments and AllowTrailingCommas options to Test-Json cmdlet #23817

iSazonov merged 4 commits into from
Jun 24, 2024

Conversation

@ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented May 19, 2024 •
edited
Loading

PR Summary

Fixes #20782

Added IgnoreComments and AllowTrailingCommas options to Test-Json -Option.

PR Context

Currently with Test-Json it does not allow comments and trailing commas.

Given the cmdlet uses System.Text.Json we can use JsonDocumentOptions to include CommentHandling and AllowTrailingCommas.

PR Checklist

iSazonov
iSazonov reviewed May 19, 2024
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I wonder why WG suggest Option as array for new parameter.
The cmdlet uses JsonNode.Parse with default JsonDocumentOption for parsing.
The JsonDocumentOption has only 3 fields and it's unbelievable that this number of parameters will be increased significantly in the future. So it would be more convenient for us to just add new individual parameters to this cmdlet as AllowTrailingCommas, CommentHandling and maybe MaxDepth.

@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented May 19, 2024

I wonder why WG suggest Option as array for new parameter.
The cmdlet uses JsonNode.Parse with default JsonDocumentOption for parsing.
The JsonDocumentOption has only 3 fields and it's unbelievable that this number of parameters will be increased significantly in the future. So it would be more convenient for us to just add new individual parameters to this cmdlet as AllowTrailingCommas, CommentHandling and maybe MaxDepth.

@iSazonov Yeah that was my thought as well. Given number of existing options is not much maybe converting all above options to switch parameters is cleaner. Not sure what the demand for MaxDepthis, and probably need to change how options are passed in since that one needs an integer.

@iSazonov iSazonov requested a review from SteveL-MSFT May 20, 2024 03:23
@iSazonov iSazonov added WG-Cmdlets general cmdlet issues CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-NeedsReview Needs a review by the labeled Working Group PowerShell-Docs needed The PR was reviewed and a PowerShell Docs update is needed labels May 20, 2024
SteveL-MSFT
SteveL-MSFT approved these changes May 20, 2024
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label May 28, 2024
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jun 5, 2024

I wonder why WG suggest Option as array for new parameter. The cmdlet uses JsonNode.Parse with default JsonDocumentOption for parsing. The JsonDocumentOption has only 3 fields and it's unbelievable that this number of parameters will be increased significantly in the future. So it would be more convenient for us to just add new individual parameters to this cmdlet as AllowTrailingCommas, CommentHandling and maybe MaxDepth.

@PowerShell/wg-powershell-cmdlets discussed this. The proposal for an array is that if newer options appear in the future, we don't end up with lots of different switches and having an array groups them making it clear these are options for non-standard JSON.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jun 5, 2024
@SteveL-MSFT SteveL-MSFT added Review - Needed The PR is being reviewed WG-Reviewed A Working Group has reviewed this and made a recommendation and removed WG-NeedsReview Needs a review by the labeled Working Group labels Jun 5, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jun 5, 2024
@ArmaanMcleod ArmaanMcleod mentioned this pull request Jun 15, 2024
Closed
2 tasks
iSazonov
iSazonov reviewed Jun 15, 2024
View reviewed changes
@ArmaanMcleod ArmaanMcleod requested a review from iSazonov June 15, 2024 23:43
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jun 24, 2024
@iSazonov iSazonov merged commit ec3840d into PowerShell:master Jun 24, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jun 24, 2024
@ArmaanMcleod ArmaanMcleod deleted the test-json-options branch June 24, 2024 08:04
@mklement0 mklement0 mentioned this pull request Jul 9, 2024
Closed
5 tasks
chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024
@ArmaanMcleod @chrisdent-de
Add IgnoreComments and AllowTrailingCommas options to Test-Json…
52642f1 
… cmdlet (PowerShell#23817)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@iSazonov iSazonov iSazonov approved these changes

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log PowerShell-Docs needed The PR was reviewed and a PowerShell Docs update is needed WG-Cmdlets general cmdlet issues WG-Reviewed A Working Group has reviewed this and made a recommendation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Breaking Change: JSONC no longer works after 7.4.0

3 participants

@ArmaanMcleod @SteveL-MSFT @iSazonov