ConvertFrom-Json: Ignore comments inside array literals (#14553)#26050
ConvertFrom-Json: Ignore comments inside array literals (#14553)#26050daxian-dbw merged 2 commits intoPowerShell:masterfrom
Conversation
MatejKafka
force-pushed
the
json-comment
branch
2 times, most recently
from
e0f3c79 to
637295e
Compare
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-Json.Tests.ps1
Outdated
Show resolved
Hide resolved
MatejKafka
force-pushed
the
json-comment
branch
from
637295e to
49175d9
Compare
iSazonov
added
the
CL-General
|
@MatejKafka |
|
/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
LGTM with one style comment.
| It 'Ignores comments in arrays' -TestCase $testCasesWithAndWithoutAsHashtableSwitch { | ||
| param($AsHashtable) | ||
|
|
||
| # https://github.com/powerShell/powerShell/issues/14553 |
There was a problem hiding this comment.
@MatejKafka Please remove. We never use references to GitHub in tests.
There was a problem hiding this comment.
Hmm, I think there is no harm to have this reference in test :)
There was a problem hiding this comment.
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
bot
added
the
Waiting on Author
daxian-dbw
dismissed
iSazonov’s stale review
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
added
CL-General
|
📣 Hey @@MatejKafka, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
| // This function is a clone of PopulateFromList using JArray as input. | ||
| private static ICollection<object> PopulateFromJArray(JArray list, out ErrorRecord error) | ||
| { | ||
| error = null; |
There was a problem hiding this comment.
@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
bot
removed
the
Waiting on Author
…rShell#14553) (PowerShell#26050) Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
4 participants
PR Summary
Fixes #14553.
PR Context
Previously,
ConvertFrom-Jsonturned comments inside JSON arrays into strings, so callingresulted in
@(" comment", 100). This PR fixes the issue by correctly ignoring comments.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header