JSON5 Support #152

dhilst · 2022-08-13T19:43:55Z

Hello,

This MR creates a Yojson_json5 library on top of Yojson adding support for JSON5, it's based on the work of @gorm-issuu and @gertsonderby

It's regarding #106

I have a primary interface for parsing strings, files, and channels to Yojson.Safe.t Yojson.Basic.t and Yojson.Raw.t, the comments are dropped. OCaml is new to me I would like to ask for help with the next steps.

Leonidas-from-XIV

Hi @dhilst. Thanks for your effort! It's quite a lot of code, so I expect we'll need some iterations on that. I took a first look through the code and wrote some suggestions on how to improve. I haven't tried the code yet, so this is just a cursory view with my impressions.

Since we're not using CPPO in this part of the codebase, I think it would be reasonable to add OCamlformat support to it, disable it and only enable it in the new part of the codebase, so we won't have to discuss style and formatting. I have found the experience of not dealing with that save a lot of time and nerves (and also being able to write code in any way and then have ocamlformat make it look good was useful).

Looking at the CI it looks that some code is failing on older versions. Given this PR uses sedlex we have to restrict the minimum version of this subpackage to OCaml 4.08 (which I think is completely ok) and make sure the library and tests are only part of the yojson_json5 package (this can be done in the dune files). That should solve at least some of the minimum version failures.

I remember @gorm-issuu had some discussions about escape sequences in strings with @andersfugmann. Gorm, do you remember what the point in question was?

lib/json5/ast.ml

lib/json5/basic.ml

lib/json5/ast.ml

lib/json5/dune

lib/json5/ast.ml

yojson_json5.opam

test/json5/dune

lib/json5/lexer.ml

lib/json5/parser.ml

dhilst · 2022-08-15T12:22:58Z

@Leonidas-from-XIV thank you for the review, I will address the comments and let you know when it's ready for review again!

andersfugmann · 2022-08-16T06:54:17Z

I do vaguely remember some discussions about escape sequences, but forgot the details.
Looking briefly at the PR, I cannot help to wonder if ocamllex + menhir would be better suited to replace (auto generate) the lexer and parser modules. It would add readability and maintainability.

Leonidas-from-XIV · 2022-08-16T08:04:33Z

The problem with ocamllex is that it is not unicode-aware and the JSON5 spec specifies that the sequences are unicode. I am sure this can be hacked in ocamllex, but the advantage of using sedlex in this case is that the lexer can be basically lifted out of the JSON5 specification with all the Unicode codepoints specified, instead of attempting to match on potentially multi-byte characters.

Also, ocamllex has the disadvantage that the mll files are not OCaml, whereas sedlex files are just OCaml with PPX, so tools like Merlin/OCaml-LSP or OCamlformat can be used on them as normal.

dhilst · 2022-08-20T14:12:21Z

Hi @Leonidas-from-XIV I would like to ask for another review, I addressed the comments.

Something that caught my attention is dune runtests -p yojson_json5 do not run the tests in the test/json5 folder, I'm new to dune and OCaml in general so I don't know that would be the right approach here, if I understand right yojson is the package (defined in the dune-project file). How we want to delivery this, as another namespace inside Yojson or as another library that depends on yojson?

dhilst · 2022-08-20T17:34:23Z

~~About ocamlformat, I need to change the dune lang version from 1.0 to 1.4 to get rid of this warning~~

File "dune-project", line 3, characters 11-14:
3 | (using fmt 1.0)
               ^^^
Warning: Version 1.0 of integration with automatic formatters is not
supported until version 1.4 of the dune language.
There are no supported versions of this extension in version 1.0 of the dune
language.

dhilst · 2022-08-20T17:38:17Z

Oh it seems that I need to rebase, the master has dung-lang > 2.0 so I should get formating for free by rebasing

dhilst · 2022-08-20T19:49:11Z

I have this error on the CI

File "test/json5/dune", line 3, characters 21-33:
3 |  (libraries alcotest yojson_json5))
                         ^^^^^^^^^^^^
Error: Library "yojson_json5" not found.
-> required by
   _build/default/test/json5/.json5_test.eobjs/byte/dune__exe__Json5_test.cmi
-> required by alias test/json5/check

I think this is because the json5 folder is inside test/, I could fix that by splitting the test folders in two test/yojson and test/yojson_json5 and running the tests like this dune runtest -p yojson test/yojson. I didn't push this because it moves a lot of files and generate a lot of noise the MR, and maybe there is an easier/better way to solve this error, it's stashed locally in any case.

Leonidas-from-XIV · 2022-08-22T10:37:34Z

Oh it seems that I need to rebase, the master has dung-lang > 2.0 so I should get formating for free by rebasing

Yes, also if you take a look at this PR you'll see ocamlformat being enabled (since I realized we can already enable it for the tests). I hope to merge it soon, so you'll be able to rebase on master and get the config and all set up.

Leonidas-from-XIV · 2022-08-22T10:57:18Z

I checked out the tests and they are not being run, so I needed to add this

(rule
 (alias runtest)
 (package yojson_json5)
 (action (run ./json5_test.exe)))

At which point the tests ran successfully.

The failures in the CI are happening on older versions where yojson_json5 can't be built, yet it tries to build the tests. You need to constrain the tests to the yojson_json5 package, so it would (hopefully) not attempt to build them on these versions.

Leonidas-from-XIV

Some more tips & suggestions. Sorry for the parallel work going on in the other PRs which might need to some rebasing once they're done, but I hope it shouldn't be too bad.

.ocamlformat-enable

lib/json5/lexer.ml

lib/json5/parser.ml

lib/json5/read.ml

Leonidas-from-XIV · 2022-08-22T12:05:42Z

lib/json5/yojson_json5.mli

@@ -0,0 +1,101 @@
+module Safe : sig
+  type t = Yojson.Safe.t
+  type result := (t, string) Result.t


I would try to avoid this alias, in case people unwisely do open since it clashes with the built-in ('a, 'e) result type.

Leonidas-from-XIV · 2022-08-22T12:17:53Z

test/json5/json5_test.ml

+end
+let yojson_json5 = Alcotest.testable M.pp M.equal
+
+let test_pp () =


This appears to be unused?

lib/json5/ast.ml

Leonidas-from-XIV · 2022-08-22T12:27:32Z

lib/json5/lexer.ml

+
+let escape_sequence =
+  [%sedlex.regexp? character_escape_sequence | '0' | unicode_escape_sequence]
+(* TODO *)


There is still this TODO.

I remove the TODO it seems to be remaining.

dhilst · 2022-08-22T13:34:52Z

Thanks for the review @Leonidas-from-XIV I will address the comments and let you know when it's ready for another review round!

dhilst · 2022-08-28T12:16:03Z

Hello @Leonidas-from-XIV, this PR is ready for review again.

If the code is okay I think I can focus on doing more tests in the next round, what do you think?

Leonidas-from-XIV

Thanks! Looks like we're reaching the home stretch. I looked up the TODO in the spec and found a missing part (that would be nice to add as a test), but apart from that it's mostly cosmetic changes.

dune-project

lib/json5/ast.ml

lib/json5/parser.ml

lib/json5/read.ml

test_json5/test.ml

lib/json5/lexer.ml

dhilst · 2024-03-25T23:43:44Z

Hello everyone. I'd like to know what the status of this MR and if it will be merged soon, if someone knows.

This looks really interesting and I would really enjoy to be able to parse JSON5 files in my projects, in particular files containing trailing commas.

Many thanks!

I fixed the conflict!

You can try it by pinning the branch with opam, you can check the test_json5/test.ml for examples. As far as I remember the API is the same.

Maybe this is a good time to talk about documentation as I didn't touch the documentation at all

c-cube · 2024-03-26T16:43:11Z

I looked at the code and it looks pretty good. I can't comment on the sedlex part because I don't know sedlex, but it'd be good to get JSON5 support and not delay this much further imvho.

Leonidas-from-XIV · 2024-04-26T14:04:48Z

@dhilst I tried pushing some fixes on top of your branch but I don't have the permission. Could you contact GH to reroute your fork from issuu/yojson to ocaml-community/yojson? They have a form for that, in the meantime I've temporarily created #176 with my changes.

dhilst · 2024-04-29T10:58:09Z

@dhilst I tried pushing some fixes on top of your branch but I don't have the permission. Could you contact GH to reroute your fork from issuu/yojson to ocaml-community/yojson? They have a form for that, in the meantime I've temporarily created #176 with my changes.

Done!

CHANGES.md

The lexer should return a string, no need to add double or single quotes when lexing, they are not the content of the string.

Leonidas-from-XIV · 2024-05-30T14:11:51Z

Looks like the remaining CI failures are unrelated to this PR, fixed the multiple dangling comma issue, so I think this is ready to go. Thanks to @dhilst @gertsonderby @gorm-issuu @c-cube for contributing and reviewing. I think this should be good to be released as part of Yojson 2.2.

panglesd · 2024-05-31T08:14:05Z

Thanks a lot to everyone involved in this PR! This will be a huge quality of life improvement for dealing with handwritten json config files!

@dhilst

CHANGES: *2024-05-31* ### Added - Added support for JSON5 (@dhilst, @gorm-issuu, @gertsonderby, ocaml-community/yojson#152) ### Removed - Remove CPPO dependency to make the Yojson installation lighter (@Leonidas-from-XIV, ocaml-community/yojson#175)

@dhilst

CHANGES: *2024-05-31* ### Added - Added support for JSON5 (@dhilst, @gorm-issuu, @gertsonderby, ocaml-community/yojson#152) ### Removed - Remove CPPO dependency to make the Yojson installation lighter (@Leonidas-from-XIV, ocaml-community/yojson#175)

dhilst requested review from NathanReb, panglesd and Leonidas-from-XIV as code owners August 13, 2022 19:43

Leonidas-from-XIV requested changes Aug 15, 2022

View reviewed changes

dhilst force-pushed the json5-parser-implementation branch from 2c88737 to e52402d Compare August 20, 2022 14:44

dhilst force-pushed the json5-parser-implementation branch from 6005f94 to 7cfc654 Compare August 20, 2022 17:43

Leonidas-from-XIV requested changes Aug 22, 2022

View reviewed changes

dhilst force-pushed the json5-parser-implementation branch 2 times, most recently from da4a0f1 to cf03600 Compare August 28, 2022 12:13

Leonidas-from-XIV requested changes Aug 29, 2022

View reviewed changes

dhilst force-pushed the json5-parser-implementation branch 9 times, most recently from f824843 to 8e77ded Compare September 4, 2022 17:46

Add more tests for JSON5

59659e2

dhilst force-pushed the json5-parser-implementation branch from 703d505 to 59659e2 Compare March 25, 2024 23:38

Leonidas-from-XIV mentioned this pull request Apr 26, 2024

JSON5 support (PR with fixes) #176

Closed

Leonidas-from-XIV self-assigned this May 6, 2024

Leonidas-from-XIV added the feature request label May 6, 2024

dhilst commented May 21, 2024

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

Leonidas-from-XIV added 8 commits May 30, 2024 11:45

Update Changelog

01e30dd

Extend tests and split them into their own test cases

3d40870

Fix issue with quoting

afcb738

The lexer should return a string, no need to add double or single quotes when lexing, they are not the content of the string.

Simplify the code with Result.map operators

6514db3

Add test cases for trailing commas

e976079

Unify all tests to use Alcotest.result to compare

fd07bd7

Fix handling of multiple trailing commas

dbe0b81

More tests and better argument order

5a81430

Leonidas-from-XIV force-pushed the json5-parser-implementation branch from 0dc2ce4 to 5a81430 Compare May 30, 2024 09:45

Do not depend on fmt

1df1df5

Leonidas-from-XIV approved these changes May 30, 2024

View reviewed changes

Leonidas-from-XIV merged commit 2f1d378 into ocaml-community:master May 30, 2024
3 of 4 checks passed

Leonidas-from-XIV mentioned this pull request May 31, 2024

Discussion / Draft: Support trailing commas for JSON parsing #87

Closed

Leonidas-from-XIV mentioned this pull request May 31, 2024

[RFC] Support for JSON5 parsing #106

Closed

Leonidas-from-XIV mentioned this pull request May 31, 2024

[new release] yojson and yojson-five (2.2.0) ocaml/opam-repository#25994

Merged

sim642 mentioned this pull request Jun 24, 2024

JSON5 conf files goblint/analyzer#1524

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON5 Support #152

JSON5 Support #152

dhilst commented Aug 13, 2022

Leonidas-from-XIV left a comment

dhilst commented Aug 15, 2022

andersfugmann commented Aug 16, 2022

Leonidas-from-XIV commented Aug 16, 2022

dhilst commented Aug 20, 2022 •

edited

Loading

dhilst commented Aug 20, 2022 •

edited

Loading

dhilst commented Aug 20, 2022

dhilst commented Aug 20, 2022

Leonidas-from-XIV commented Aug 22, 2022

Leonidas-from-XIV commented Aug 22, 2022 •

edited

Loading

Leonidas-from-XIV left a comment

Leonidas-from-XIV Aug 22, 2022

Leonidas-from-XIV Aug 22, 2022

Leonidas-from-XIV Aug 22, 2022

dhilst Aug 28, 2022

dhilst commented Aug 22, 2022

dhilst commented Aug 28, 2022

Leonidas-from-XIV left a comment

dhilst commented Mar 25, 2024

c-cube commented Mar 26, 2024

Leonidas-from-XIV commented Apr 26, 2024

dhilst commented Apr 29, 2024

Leonidas-from-XIV commented May 30, 2024

panglesd commented May 31, 2024

JSON5 Support #152

JSON5 Support #152

Conversation

dhilst commented Aug 13, 2022

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

dhilst commented Aug 15, 2022

andersfugmann commented Aug 16, 2022

Leonidas-from-XIV commented Aug 16, 2022

dhilst commented Aug 20, 2022 • edited Loading

dhilst commented Aug 20, 2022 • edited Loading

dhilst commented Aug 20, 2022

dhilst commented Aug 20, 2022

Leonidas-from-XIV commented Aug 22, 2022

Leonidas-from-XIV commented Aug 22, 2022 • edited Loading

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Leonidas-from-XIV Aug 22, 2022

Choose a reason for hiding this comment

Leonidas-from-XIV Aug 22, 2022

Choose a reason for hiding this comment

Leonidas-from-XIV Aug 22, 2022

Choose a reason for hiding this comment

dhilst Aug 28, 2022

Choose a reason for hiding this comment

dhilst commented Aug 22, 2022

dhilst commented Aug 28, 2022

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

dhilst commented Mar 25, 2024

c-cube commented Mar 26, 2024

Leonidas-from-XIV commented Apr 26, 2024

dhilst commented Apr 29, 2024

Leonidas-from-XIV commented May 30, 2024

panglesd commented May 31, 2024

dhilst commented Aug 20, 2022 •

edited

Loading

dhilst commented Aug 20, 2022 •

edited

Loading

Leonidas-from-XIV commented Aug 22, 2022 •

edited

Loading