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 .jsonc support to ConvertFrom-Json #7436

Closed
chriskuech opened this issue Aug 2, 2018 · 19 comments
Closed

Add .jsonc support to ConvertFrom-Json #7436

chriskuech opened this issue Aug 2, 2018 · 19 comments
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-Answered The question is answered. WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module

Comments

@chriskuech
Copy link

"JSON with Comments" with the jsonc file extension is a natively supported file format in VS Code. JSONC adds JS-style comments to JSON files. ConvertFrom-Json currently does not support parsing JSONC strings. JSONC support could be added through either a -Comments switch or by default.

Background

Azure Resource Manager PowerShell and Azure Resource Manager templates are a critical component of the Azure ecosystem; many templates provided by Azure are large and complicated JSON files with over 1000 lines. Azure Resource Manager PowerShell cmdlets currently support JSONC templates, but cannot be used in conjunction with custom PowerShell validation due to the lack of JSONC support in ConvertFrom-Json

@iSazonov iSazonov added Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module labels Aug 3, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Aug 3, 2018

/cc @markekraus

@markekraus
Copy link
Contributor

@chriskuech Can you provide some example JSON, example PowerShell code, what you expect, and what you currently see?

NewtonSoft supports working with JSON comments but has 2 modes: ignore and load. Though, with how the cmdlets currently operate, it may not be possible to make use of that.

@chriskuech
Copy link
Author

Working base case

"{`"hello`": `"world`", `n`"a`": 2}" | ConvertFrom-Json

Single-line comment

"{`"hello`": `"world`", // a common example `n`"a`": 2}" | ConvertFrom-Json
ConvertFrom-Json : Invalid object passed in, ':' or '}' expected. (20): {"hello": "world", // a common example
"a": 2}
At line:1 char:60
+ ... llo`": `"world`", // a common example `n`"a`": 2}" | ConvertFrom-Json
+                                                          ~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [ConvertFrom-Json], ArgumentException
    + FullyQualifiedErrorId : System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand

Multi-line comment

"{`"hello`": `"world`", /* comment test */ `n`"a`": 2}" | ConvertFrom-Json
ConvertFrom-Json : Invalid object passed in, ':' or '}' expected. (20): {"hello": "world", /* comment test */
"a": 2}
At line:1 char:59
+ ... ello`": `"world`", /* comment test */ `n`"a`": 2}" | ConvertFrom-Json
+                                                          ~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [ConvertFrom-Json], ArgumentException
    + FullyQualifiedErrorId : System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand

Expectation

Comments are ignored and only the value is returned

hello a
----- -
world 2

@johlju
Copy link

johlju commented Aug 20, 2018

This would be a great addition to an already well used cmdlet, able to import a .jsonc file and convert it to an object as usually.

'{
    // Comment
    "property1": "value1"
}' | ConvertFrom-Json

To use ConvertFrom-Json now, the file first needs to loaded and then be stripped from comments.

@markekraus
Copy link
Contributor

markekraus commented Aug 20, 2018

One argument against supporting JSON comments is that they are not RFC defined. That makes it difficult to write code that supports something that is not well defined. The general guidance is to not use comments in JSON that are not valid JSON. Instead, adding a metadata object or a "_comment" property is recommended.

If we did support this, we would not be able to include it in the default operation of ConvertFrom-Json until comments for JSON were RFC-backed. It would require a Switch of some kind. -SkipComments or something. That is because we need to reserve the default operation of the cmdlet for RFC compliance. If a later RFC implements comments as beginning with #, for example.

Also, what do we do with the comment data? Nothing? Apply it as a property on the object it comments?

@iSazonov
Copy link
Collaborator

Maybe delegate this to Newton.Json? Seem the library is a standard by default.

@johlju
Copy link

johlju commented Aug 20, 2018

@markekraus I would say make the use of comments opt-in. Default is today, it will fail parsing the file. If using -AllowComments then it can parse the file (using non-RFC backed way). It would simplify when it is RFC backed. Then the -AllowComments would be obsolete, and the default is to be able to parse both.

I suggest we do nothing with comments, the comments are comments for the jsonc file, not part of the object or data, unless and RFC say differently in the future.

@markekraus
Copy link
Contributor

@iSazonov Delegating to Newtonsoft is a technical implementation, I'm more concerned about design at this point. I think supporting jsonc probably deserves a PowerShell-RFC.

@chriskuech
Copy link
Author

For the use case I mentioned, using metadata objects and "_comment" properties would not suffice, as it would break conformity with the external schema.

@markekraus
Copy link
Contributor

@chriskuech I understand. I'm just pointing out that JSONC is not an RFC backed spec and as such the general guidance is to either not use it or to add JSON compliant comments and metadata where possible. And I'm only establishing that as a reason why it should not be included in the default behavior of the cmdlet.

@erictorbenson
Copy link

I think @johlju has the right idea about adding an "-AllowComments" switch. That way the cmdlet defaults to RFC-compliant behavior...unless someone wants to write a new RFC? :-)

One very important argument in favor of supporting comments is the Visual Studio "Azure Resource Group" sample project. The PowerShell wrapper that uploads the artifacts to Azure Storage and executes the deployment fails to run when one of the templates or parameters has a comment in it. I stripped out all the comments from the file manually and everything was fine -- with them there, the error that @chriskuech received is what stops the deployment. If I had an "-AllowComments" switch, I could just modify the wrapper script and there would be no problem.

ARM templates can become very confusing and stretch to thousands of lines, and not everyone is 100% dedicated to automated pipeline-based deployment yet. Putting a set of comments in to clearly mark what needs to be modified by a human reduces the chance for error.

@SteveL-MSFT SteveL-MSFT added this to the 6.3-Consider milestone Feb 26, 2019
@joeyaiello
Copy link
Contributor

Yeah, this seems very useful to me. -AllowComments is probably the way to go (for the reasons @markekraus pointed out).

@TravisEz13
Copy link
Member

TravisEz13 commented Feb 26, 2019

Comments are allowed by default

'"test" /*test*/' | convertfrom-json

test

'"test" //test' | convertfrom-json

test

image

Name                           Value
----                           -----
PSVersion                      6.1.3
PSEdition                      Core
GitCommitId                    6.1.3
OS                             Darwin 18.2.0 Darwin Kernel Version 18.2.0: Thu Dec 20 20:46:53 PST 2018; root:xnu-4903.241.1~1/RE...
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

@adityapatwardhan
Copy link
Member

I have validated on PowerShell Core 6.1.3 on Windows:

Single line comment:

PS> "{`"hello`": `"world`", // a common example `n`"a`": 2}" | ConvertFrom-Json                                                                                                                                                              
hello a
----- -
world 2

Multi-line comment:

PS> "{`"hello`": `"world`", /* comment test */ `n`"a`": 2}" | ConvertFrom-Json                                                                                                                                                               
hello a
----- -
world 2
PS> $PSVersionTable                                                                                                                                                                                                                          
Name                           Value
----                           -----
PSVersion                      6.1.3
PSEdition                      Core
GitCommitId                    6.1.3
OS                             Microsoft Windows 10.0.18845
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

@markekraus
Copy link
Contributor

awesome. It has probably been like this since 6.0.0 and we just never validated the premise that they didn't work in Core.

@TravisEz13 TravisEz13 added the Resolution-Answered The question is answered. label Feb 26, 2019
@TravisEz13
Copy link
Member

Verified with @chriskuech the question was answered.

@msftrncs
Copy link
Contributor

@SteveL-MSFT, I think the tag 7.0-consider is no longer needed. Seems this is already implemented.

@ChesedPrather
Copy link

Will the ability to simply comment-out (block-out) sections of code like is available in every other format XML , PS1 <##>, JAVA, CPP, etc /* */. Is this feature ever coming or should we look elsewhere?

@goyalyashpal
Copy link

goyalyashpal commented May 10, 2024

Comments are allowed by default

'"test" /*test*/' | convertfrom-json
'"test" //test' | convertfrom-json
Name                           Value
----                           -----
PSVersion                      6.1.3
PSEdition                      Core
OS                             Darwin 18.2.0 Darwin Kernel Version 18.2.0: Thu Dec 20 20:46:53 PST 2018; root:xnu-4903.241.1~1/RE...
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
WSManStackVersion              3.0

- @ TravisEz13 at #7436 (comment)

just for my own reference, it doesn't work on 5.1.19041.4291 (via echo.exe $PSVersionTable.PSVersion) on my win10 22h2 pc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-Answered The question is answered. WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module
Projects
None yet
Development

No branches or pull requests