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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArmaanMcleod
Copy link
Contributor

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

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.

[Parameter]
[ValidateNotNullOrEmpty]
[ValidateSet("IgnoreComments", "AllowTrailingCommas")]
public string[] Option { get; set; } = Array.Empty<string>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does ValidateSet really work for array?

Copy link
Member

Choose a reason for hiding this comment

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

@ArmaanMcleod
Copy link
Contributor Author

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
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

[Parameter]
[ValidateNotNullOrEmpty]
[ValidateSet("IgnoreComments", "AllowTrailingCommas")]
public string[] Option { get; set; } = Array.Empty<string>();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Review - Needed The PR is being reviewed WG-Cmdlets general cmdlet issues WG-NeedsReview Needs a review by the labeled Working Group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking Change: JSONC no longer works after 7.4.0
3 participants