Add support for JSON comments - #1117
Open
Kira-NT wants to merge 3 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Hi! This PR adds optional support for JSON-style comments.
YAML 1.2 is, by design, a superset of JSON. Unfortunately, though, with the widespread adoption of formats like JSONC, the ability to use a single YAML parser for both YAML and JSON has become less relevant over time. In my case, for example, I'm working on a tool that I want to keep as small as possible when compiled with NativeAOT to simplify its distribution. However, it needs to parse both YAML and JSON with comments, which forces me to depend on both
System.Text.JsonandYamlDotNet- even though I'd happily sacrifice some performance when parsing strictly JSON files if it meant I could trimSystem.Text.Jsonaway.This PR should help people in similar situations by allowing
YamlDotNetto process comments according to the JSONC specification, if they choose to enable the feature.Usage
Compatibility mode for JSON-style comments is enabled by setting
Scanner.AllowJsonCommentstotrue.You may notice that this property is marked as
internal. My reasoning is that, since this feature has nothing to do with the YAML specification, it's not really fair to ask theYamlDotNetdevelopers to support it. At the same time, making the flag part of the public API would implicitly commit them to doing so. For that reason, I madeAllowJsonCommentsinternal, as this leaves room for the maintainers to modify or even remove the feature entirely if it ever becomes too burdensome, without having to treat the change as a breaking one.Users who want to opt into this feature can do so via:
Or, better yet:
That said, I may be overthinking this a bit, and perhaps simply marking the property with
[EditorBrowsable(EditorBrowsableState.Never)]would communicate the same intent without it being so overzealous. Let me know what you think!Example
When
AllowJsonCommentsis enabled,YamlDotNetbecomes capable of parsing any JSONC file (and even JSONC embedded within YAML). Here's an example taken directly from the new tests:{ /* * Top comment. */ /*0*/ /*1*/"foo"/*2*/ /*3*/:/*4*/ /*5*/false/*6*/ /*7*/,// 8 // Middle comment. /*9*/"bar"/*10*/:/*11*/true//12 /* * Bottom comment. */ }By default, it's parsed as:
{ "/* * Top comment. */ /*0*/ /*1*/\"foo\"/*2*/ /*3*/:/*4*/ /*5*/false/*6*/ /*7*/": null, "// 8 // Middle comment. /*9*/\"bar\"/*10*/:/*11*/true//12 /* * Bottom comment. */": null }With
AllowJsonCommentsset totrue, however, it becomes:{ "foo": false, "bar": true }Implementation
When
AllowJsonCommentsisfalse(i.e., the default), the logic ofScannerremains completely unaltered.However, funnily enough, even when I set it to
trueby default and ran the test suite to see which YAML features would break in that mode, all tests still passed. Obviously, tests are imperfect, but hopefully this gives you an idea of how unintrusive this feature is.As for the notable differences in parsing behavior, consider the following example:
By default,
YamlDotNetparses it as:{ "foo": "https://github.com", "bar": "null/*null*/", "baz": "baz/*baz*/", "// one": "1", "// two": null }With
AllowJsonCommentsenabled, it becomes:{ "foo": "https://github.com", "bar": null, "baz": "baz/*baz*/" }This is definitely not something you would typically see in the real world, so it is unsurprising that the current test suite does not catch these differences. Nevertheless, it is still worth noting that they exist.
Remaining Questions
AllowJsonCommentsremaininternal?Closes #1052
Closes #1112