[System.Text.Json] Add ability to serialize Comments on Properties

[nikeee]

System.Text.Json already supports comments during deserialization.

JSON config files of VSCode and TypeScript and other programs use comments on their properties to add some in-line documentation.

For programs that use JSON as a config format, it would be nice if they could just annotate their classes and have these annotations as comments in the serialized JSON. This would be useful to generate a default/initial configuration file.

For example, consider:

class Root
{
    [JsonPropertyName("test")]
    public float test = 0.42f;

    [JsonPropertyName("growIncrement")]
    [JsonComment("Lorem Ipsum")]
    public float GrowIncrement = 0.42f;

    public string test2 = "foo";
}

Could serialize to something like this:

{
    "test": 0.42,
    // Lorem Ipsum
    "growIncrement": 0.42,
    "test2": "foo"
}

or this:

{
    "test": 0.42,
    "growIncrement": 0.42, // Lorem Ipsum
    "test2": "foo"
}

Of course, multi-line comments could be used instead of single-line comments (just mimicking VSCode's config here).

If not baked directly into the API, it would be nice if we had the ability to add this comment serialization as a separate library.

There is some need for the feature according to this SO question: https://stackoverflow.com/questions/9495718 and JamesNK/Newtonsoft.Json#1211

[ghost]

Tagging subscribers to this area: @jozkee
Notify danmosemsft if you want to be subscribed.

[FyiurAmron]

No. That's not "JSON". It's a JSON-like format. As long as JSON has official spec, and something doesn't conform to that spec, it's not "JSON" by any means. Use jsonc, hjson, JSON5, YAML, XML, or whatever you like instead. Don't expect the only stable and sensible data format in the wild to break just because you can't understand how to use it. See https://stackoverflow.com/questions/244777/can-comments-be-used-in-json , https://nlohmann.github.io/json/features/comments/ etc. for related discussion.

[nikeee]

This is a valid feature request that (according to the votes) other users also need.

No. That's not "JSON". It's a JSON-like format

That's right. As long as it's opt in (and that's what I am proposing here), the default behavior conforms with "the" standard. The parser already has support for comments in deserialization (which is also not "JSON compliant"). This feature request just asks to extend that support.

Also, notice my comment requesting this feature as a separate library if it doesn't make it into the core APIs. That would be fine, too.

because you can't understand how to use it

Please be polite. I didn't do anything to you. We're all just trying to create APIs that make us developers happy and more productive.
You may also take a look at the code of conduct of this repository.

[FyiurAmron]

Also, we already have #29543 , #30608 etc. Adding Yet Another Obscure Case To Handle is IMVHO not only asking for trouble, but outright demanding it.

[eiriktsarpalis]

I concur with @FyiurAmron's comments. It's unlikely such a design would be accepted.

[nikeee]

@eiriktsarpalis
How about the respective primitives, so that 3rd party libraries could implement that (which I also proposed)? Or is that possible today?

A lot of projects are currently dichting Newtonsoft.JSON, since that project is basically abandoned in favor of System.Text.Json. Pulling in an entirely separate JSON parsing library would be counter-productive and may cause dependency problems (which is one of the reasons why ASP.NET ditched Newtonsoft.JSON).

It's fine closing this issue when it doesn't align with the project's goals.

I'm also open for recommendations from @FyiurAmron for libraries that support that proposed feature, since the problem hasn't gone away.

[eiriktsarpalis]

It might be possible in the future via #60518. We're considering implementing this in 7.0.0

[nikeee]

Does this have a similar API surface as Newtonsoft's JSON? If I get this right, this would not offer the ability to implement the feature proposed here (feel free to correct me). More details in this issue: JamesNK/Newtonsoft.Json#1211

[FyiurAmron]

@nikeee

How about the respective primitives, so that 3rd party libraries could implement that (which I also proposed)? Or is that possible today?

No, it's not possible, since current design consists of tightly-coupled classes, with virtually no way to inject 3rd-party behaviour in the (de)serializer. It's borderline possible via reflection and assembly hacking, but I assume that's not what you want.

Pulling in an entirely separate JSON parsing library would be counter-productive

Once again, you're barking at a wrong tree. Please, for the sake of the argument, read what I wrote at least once with actual understanding on your side. It's not a "JSON parsing library" once you ditch the RFC. You require something that is not a JSON library. I'm not arguing you shouldn't do it. Do it, if it benefits you. Just be aware that using a non-standard format means you'll have to support it yourself, and that may (or may not) be counter-productive.

As far as I know, support for emitting comments was omitted (even if intended at some point, at least looking at the docs & code) from Utf8JsonWriter - it would make the code even more complicated & byzantine than it is now. Amount of classes that would have to be modified to support it ATM is easily reaching 10 (virtually entire serializer stack), unless you want to do some dirty hacks for it.

First, Utf8JsonWriter emits the separator when writing the next property, not the current one - that logic would have to be changed to allow emitting comments the way you want (and it's not that trivial of a change, even if it would seem so, if you'll check the code). Second, Utf8JsonWriter would have to have a completely new set of WriteEndXXX() functions, parametrized with your optional comment (or maybe have WriteEndComment or something similar), and that function(s) would have to be abstracted to those JsonConverters that possibly would be able to work with the data entities you want to serialize (basically, at least this Converter, possibly other ones) and to extract the data from the attribute. And that's just the beginning of it, with no spec from you how to e.g. handle nested comments, handle multi-line comments, how to handle comment placement in writer in "minimize" mode etc.

Currently, those classes are internal (and often also sealed) for a reason. I guess you should be able to see the reason based on the code by yourself by now. Changing it without a major rewrite is currently hard-to-impossible, and that's for a good reason: current implementation was intended to cover RFC JSON and be as performant as possible, as a core lib. I understand why it wasn't intended to be extended. Doing so without sacrificing either performance and/or lots of developer-hours is impossible in this case.

If you really think this feature is worthwhile enough to warrant rewriting a lib that took, in essence, some years to complete, feel free to make a proposal to change the design of System.Text.Json classes to de-internalize the code in question (essentially: rewrite it from scratch), allowing injection via interfaces instead of current tightly-coupled design, then get enough traction and developer manpower, and voila. Honestly, I would support that notion, if only for the sake of looking at the lulz that would ensue.

I'm also open for recommendations from @FyiurAmron for libraries that support that proposed feature

I've already did that; I've already answered your question. FWIW, did you wonder why e.g. JSON5 for C# was abandoned?

(Hint: yes, it's because it's a big and non-trivial piece of code, and it has to be maintained, and nobody wanted to do it)

[nikeee]

I've already answered your question

Sorry, I seem to have overlooked it. Json5 doesn't work for my use case, but maybe it will work for others reading this issue later, thanks!
Could you give post some pointers to the other alternatives?

And that's just the beginning of it, with no spec from you how to e.g. handle nested comments, handle multi-line comments, how to handle comment placement in writer in "minimize" mode etc.

When using System.Text.Json's APIs/primitives, that kind of stuff would be a problem of the implementor, not this project, if this would be possible to do so.

Just be aware that using a non-standard format means you'll have to support it yourself,

Well, skipping comments is implemented, they are non-standard as well.
Don't get me wrong here, but IMHO "it's not compliant to the standard" may not be a good argument because of this. "It's complicated to implement and maintain" is one, as well as "we favor performance over flexibility and extendability".

On the companion issue of Newtonsoft's JSON, it was mentioned that "implementing it ATM is trivial", so I assumed that it would be here, too due to some similarities in the design.

Once again, I understand that this may not align with the project's goals. No need to discuss that any further.
I apologize if something could be interpreted as mean on my end (not a native speaker here).

[FyiurAmron]

@nikeee

Could you give post some pointers to the other alternatives?

sigh I'll be repeating myself here (at least partially), but that can't be reasonably avoided or expected to be avoided.

https://github.com/hjson/hjson-cs
https://github.com/aaubry/YamlDotNet (yes, YAML is a superset of JSON, you can parse regular JSONs with YAML parser [with minor caveats])
https://github.com/d3x0r/JSON6 (you'd have to wrap it with https://github.com/paulbartrum/jurassic or port it, sadly)
https://github.com/Microsoft/node-jsonc-parser (sadly, no C# version and no emitter)
... or simply continue using JSON.NET with your patch, possibly by forking it and maintain the fork

FWIW, I know of no RFC-conforming JSON parser that would emit comments (like I said, the producer is bound to be more strict than the consumer; lax parsers are commonplace to handle the abuse of the standard in the wild).

if this would be possible to do so.

IMVHO, in System.Text.Json - not without a major rewrite, no.

Well, skipping comments is implemented, they are non-standard as well.

https://seriot.ch/projects/parsing_json.html

Decision to accept a non-conforming input that is semi-common is a result of a conscious compromise, not an argument that the serializer "[does] not [have to be] compliant to the standard".

On the companion issue of Newtonsoft's JSON, it was mentioned that "implementing it ATM is trivial",

Yes, in JSON.NET it actually is/was, as you already know.

so I assumed that it would be here, too due to some similarities in the design.

That's the one assumption that went too far. The interface is similar (since that was the intention), the implementation, arguably, is not.

I apologize if something could be interpreted as mean on my end (not a native speaker here).

Just please remember, that there are also different approaches to software engineering ฅ ̳͒•ˑ̫• ̳͒ฅ

[nikeee]

sigh I'll be repeating myself here

Well, except the reference to Yaml, all of the link you posted were never mentioned before in any related thread.

https://github.com/aaubry/YamlDotNet

Using a YAML parser to parse JSON would implicitly allow the input to be YAML, which is not what we tried to achieve in this feature request. There are reasons to avoid YAML for configuration files. Unless that YAML parser can be restricted to only allow JSON, that would not solve the problem. Also, the comment sign # is different than in what is normally used for comments in JSON files (////**/). That JSON files couldn't even be processed with the common tools, since # as a line comment in JSON is even more non-standard than comments.
Does this YAML library allow serialization of comments, like it was tried to achieve in this FR?

https://github.com/hjson/hjson-cs https://github.com/d3x0r/JSON6

Basically the same story as for YAML. HJSON (like JSON5 and JSON6) is very uncommon and not even supported in most editors (unlike JSON/JSONC), which is mandatory for use in config files.
Also, do these libraries support annotating properties with comments in serialization?

https://github.com/Microsoft/node-jsonc-parser

That would actually solve the problem, since thats basically what this FR (and the companion issue in Newtonsoft.JSON) is all about. Sadly, as you said, it is not written in C# and must basically be re-written in its entirety. (De)serializing stuff in JS is much simpler due to the dynamic type system, so this cannot be ported over 1:1 to C#.
And since the original version doesn't even have an emitter, it doesn't actually help with the current problem in any way, since this FR is solely about emitting.

... or simply continue using JSON.NET with your patch

I'd do that, but that fork/patch/draft PR doesn't even implement the feature how it was proposed. It's open for comments, though.

Just please remember, that there are also different approaches to software engineering ฅ ̳͒•ˑ̫• ̳͒ฅ

That must be why this repository has a code of conduct that prohibits such "approaches".

[FyiurAmron]

@nikeee

Well, except the reference to Yaml, all of the link you posted were never mentioned before in any related thread.

They were, if you'd kindly spend 5 minutes using Google.

That must be why this repository has a code of conduct that prohibits such "approaches".

AFAIK, no line in .NET CoC prohibits sense of humour. I had already advised you on that on numerous occasions, and I'd suggest you take that suggestion, instead of facilitating the "broken record" approach.

For me, this discussion has run its course.