KEMBAR78
ConvertFrom-Json: Ignore comments inside array literals (#14553) by MatejKafka · Pull Request #26050 · PowerShell/PowerShell · GitHub
Skip to content

ConvertFrom-Json: Ignore comments inside array literals (#14553) #26050

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
merged 2 commits into from
Sep 23, 2025
Merged

ConvertFrom-Json: Ignore comments inside array literals (#14553) #26050

merged 2 commits into from
Sep 23, 2025

Conversation

@MatejKafka
Copy link
Contributor

@MatejKafka MatejKafka commented Sep 14, 2025 •
edited
Loading

PR Summary

Fixes #14553.

PR Context

Previously, ConvertFrom-Json turned comments inside JSON arrays into strings, so calling

'[/* comment */ 100]' | ConvertFrom-Json

resulted in @(" comment", 100). This PR fixes the issue by correctly ignoring comments.

PR Checklist

@MatejKafka MatejKafka force-pushed the json-comment branch 2 times, most recently from e0f3c79 to 637295e Compare September 14, 2025 21:01
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 19, 2025
daxian-dbw
daxian-dbw approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM!

@daxian-dbw
Copy link
Member

daxian-dbw commented Sep 22, 2025 •
edited
Loading

@MatejKafka can you please address the 4 CodeFactor issues?
I pushed a commit to address the 4 CodeFactor issues. Please take a look. Now we need another maintainer to approve the PR :)

@daxian-dbw
Copy link
Member

/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

iSazonov
iSazonov previously requested changes Sep 23, 2025
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.

LGTM with one style comment.

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-Json.Tests.ps1
It 'Ignores comments in arrays' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
param($AsHashtable)

# https://github.com/powerShell/powerShell/issues/14553
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MatejKafka Please remove. We never use references to GitHub in tests.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think there is no harm to have this reference in test :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful if it were a tracking issue for a disabled test. In this form, it's just garbage.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 23, 2025
@daxian-dbw daxian-dbw dismissed iSazonov’s stale review September 23, 2025 17:23

I think there is no harm to have the reference to the GitHub issue in test code. Also, we are going to create the preview release branch today, so I will merge the PR with that reference.

@daxian-dbw daxian-dbw added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Sep 23, 2025
@daxian-dbw daxian-dbw merged commit 551a0fe into PowerShell:master Sep 23, 2025
38 of 39 checks passed
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Sep 23, 2025 •
edited by unfurl-links bot
Loading

📣 Hey @@MatejKafka, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

MatejKafka
MatejKafka commented Sep 30, 2025
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs
// This function is a clone of PopulateFromList using JArray as input.
private static ICollection<object> PopulateFromJArray(JArray list, out ErrorRecord error)
{
error = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daxian-dbw Just curious, why move error initialization up here? Imo initializing it at the beginning can potentially hide issues where you forget to set error before return, while if it was only assigned directly before the return, compiler would throw an error about a potentially uninitialized variable.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@iSazonov iSazonov iSazonov left review comments

@jshigetomi jshigetomi jshigetomi approved these changes

Assignees

No one assigned

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConvertFrom-Json: comments in top-level arrays are parsed incorrectly (but also are not errors)

4 participants

@MatejKafka @daxian-dbw @iSazonov @jshigetomi