Ignore trailing comma in objects and arrays

[kimsey0]

Is your feature request related to a problem? Please describe.
I use the new Windows Terminal, which uses JsonCpp for parsing JSON configuration files, and currently requires those configuration files to be manually edited. It provides a file with defaults, from which I copy paste lines with settings for individual profiles (represented as JSON objects) into my user configuration file, then change the default values. However, those setting lines end with a comma, so if I paste them at the end of an existing profile object and forget to remove the comma, JsonCpp will fail to parse the configuration file and the Windows Terminal will pop up with a "Failed to reload settings" dialog.

More generally, this problem occurs for both objects and arrays when manually constructing JSON files or copy-pasting objects from ECMAScript code, which ignores trailing commas in object and array literals.

Describe the solution you'd like
I want JsonCpp, either by default or with a configuration setting, to ignore one trailing comma in objects and arrays while deserializing. I don't expect JsonCpp to keep any reference of this trailing comma or to be able to recreate it while serializing.

Describe alternatives you've considered
The main alternative I see is to keep the current behavior and to strictly enforce the correctness of parsed JSON. However, JsonCpp already supports comments, which is non-standard, so supporting trailing commas doesn't seem at odds with the direction of the project.

Between the alternatives of ignoring a trailing comma by default or by configuration, this depends on whether JsonCpp is allowed to break backwards compatibility by parsing files that would previously have resulted in an syntax error. I don't know the project stance on this.

JsonCpp could optionally support retaining and serializing trailing commas, but I don't see any value in that, except being able to more faithfully reproduce the source when deserializing and reserializing a file.

Additional context
This feature was also requested back in 2006 on Sourceforge. @cdunn2001 back then remarked:

I see this is difficult the way readObject() and readArray() are written. Oh, well. It would be nice someday...

[kimsey0]

Issue #723 would include this if implemented.

[dota17]

I had test the setting allowDroppedNullPlaceholders_ for this case, and I set allowDroppedNullPlaceholders_ to true:

1. For Array, [1,] can be parsed, and the result is [1,null], so we have to judge the Json::nullValue.
2. For Object, allowDroppedNullPlaceholders_ is not useful.
   And I change the method OurReader::readObject()

    if (tokenName.type_ == tokenString) {
      if (!decodeString(tokenName, name))
        return recoverFromError(tokenObjectEnd);
    } else if (tokenName.type_ == tokenNumber && features_.allowNumericKeys_) {
      Value numberName;
      if (!decodeNumber(tokenName, numberName))
        return recoverFromError(tokenObjectEnd);
      name = String(numberName.asCString());
+  } else if (tokenName.type_ == tokenObjectEnd && features_.allowDroppedNullPlaceholders_) {
+    return true;
    } else {
      break;
    }

After changing, {"a":1,} can be parsed, and the result is {"a":1}.

However, when the number of the trailing commas is more than one, it still parses error.

I think the above maybe not good enough.Feel free for a PR

[cdunn2001]

This seems like a useful feature. Just please be sure that a trailing comma fails for "strict mode".

[kimsey0]

It seems to me like allowing a trailing comma in objects is quite straightforward: kimsey0@f0e7848

For arrays, an error is thrown in readValue when it reads the ] after a trailing comma. I'm looking at how to handle that nicely.

[kimsey0]

We need to determine what to do if both allowDroppedNullPlaceholders_ and allowTrailingCommas_ is enabled. Right now, with allowDroppedNullPlaceholders_, [1,] will deserialize to [1,null]. Changing that to [1] might break some users.

[kimsey0]

Also, I'm not completely sure about the distinction between Reader and OurReader. It looks like Reader is used for the tests. Should I change both?

[dota17]

@kimsey0
I think it is not a good idea to change Reader, which was deprecated.
New features should be added into OurFeature and OurReader.

[kimsey0]

@dota17: It looks like the tests in test/data are being run with Reader, since reverting the changes there breaks the tests. Do you know if there are a separate set of tests for OurReader? I can't seem to find any.

[dota17]

@kimsey0
#1053 can solve your question.
The PR add setting use_legacy in jsontestrunner so that we can check the legacy implementation Reader and the modern implementation OurReader.

[kimsey0]

@dota17: Looks good, but if we don't introduce support for trailing commas in the legacy Reader too, should we then have a way to mark test cases as modern OurReader only? Otherwise, the tests will always fail with use_legacy = true.

[dota17]

@kimsey0
Good catch.
I had added your point in #1053 .Thanks a lot.

[kimsey0]

I have created a draft pull request (#1062) so that we can also start discussions about the concrete implementation ahead of #1053 being fixed.

[dota17]

Closing as fixed in #1098