-
-
Notifications
You must be signed in to change notification settings - Fork 701
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
DRAFT: Json5 for std.json #8806
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request, @burner! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8806" |
I think this should be a separate module. Was this discussed/approved somewhere? Because I faintly recall a discussion with the consensus that this is a bad idea. Probably worth clarifying to everyone that JSON5 is not standard, it's a different language that's a superset of JSON. Someone just decided to add a few random things to JSON, and call it "JSON5". It's definitely not a "newer version of JSON". I could take the YAML spec and rename it to JSON6 with the same effect. So, adding this to Phobos will make our JSON implementation non-conformant (or if it isn't already, even more so). Maybe if it was optional? |
I like it. std.json has always been slow and non-conformant, so this will finally give it some real value |
Worth reading: |
OK, but please rename the module to |
might as well, it always seemed like std.json was kinda silently deprecated anyway. maybe that'll finally get std.data.json brought in? |
I'm going to expand on the above a bit because this is kind of frustrating to me. JSON5's name is a scam, and it successfully tricked a lot of people. Adding JSON5 to So, I think this is a bad idea, and it further supports and propagates the lie in JSON5's name. Don't get tricked! |
that is a copy paste error, will fix by the time the DRAFT is removed. This was discussed in a foundation meeting @WalterBright @atilaneves |
OK, please make sure they are aware of this discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize:
- There are many JSON supersets (YAML, ION, CUE, many others).
- JSON5 is one of many JSON supersets. It is only notable because its author had the audacity to call it "JSON5", and its provenance is completely unrelated to the original JSON language.
- JSON5 has many issues, including that it further diverges from valid JavaScript syntax, thus no longer being a subset of JavaScript.
- Choosing JSON5 as the JSON superset supported by
std.json
prevents supporting other JSON supersets instd.json
. - Other than the ill-begotten name, JSON5 does not really have any intrinsic qualities that make it the obvious choice for the JSON superset that
std.json
should support.
In short, I think this is a bad idea. We can support JSON5 in Phobos, but it should be under a new module name. However, considering that many people have a very strong negative opinion about JSON5, we should consider not supporting it at all, and instead considering a different JSON superset.
No, JSON5 means JSON with ECMAScript 5 syntax for data. The 5 corresponds to the ECMAScript revision.
I hope you can reconsider, there is a purpose behind the name, and it's not to make people think that it's a revision of JSON. However, I'm of the opinion that json5 support is not worth adding to std.json. The existing module supports a canonical format -- JSON. There are no options (aside from whitespace) when it comes to reading and writing JSON files -- everything has one representation. In order to properly support json5, you need to start adding more types to JSONType, which is going to break a lot of code. If people want to implement e.g. a json5 editor, they will not be able to use a library that throws away the other information (comments, single-quoted strings, are identifiers quoted, was hexadecimal used, etc.). To that end also, someone might want to validate that their json files are strictly json and not json5. I'll note that I actually have written a complete json5 parser/serializer in https://github.com/schveiguy/jsoniopipe/tree/master In my experience while adding this, I found that dealing with whitespace and non-quoted identifiers significantly complicates the parser. Parsing JSON only requires ASCII. Someone using std.json might not expect the most optimized performance, but still shouldn't have to pay the penalty of parsing full unicode for JSON, just in case they have actually passed JSON5 data. Given that most likely the json5 support should be added as its own file, why not a dub package instead of adding to the standard library? |
Ah, thanks; still misleading IMO!
Personally I would be more OK with this if the proposal for the format came from a more established standards body, e.g. as an annex to the ECMAScript spec. As it stands, the author still needed to make some arbitrary decisions, such as which ECMAScript features are supported and the exact syntax subset.
👍 and in line with our current policy for major Phobos additions. |
We are using std.json. Just add std.json5. Done. Great PR btw |
I rebased this PR and moved the code to std.json5. See #8875. Would that be enough to get this code merged? |
This PR adds Json5 support to std.json.
done:
todo: