yojson
yojson copied to clipboard
ocaml-community
JSON5 Support
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 https://github.com/ocaml-community/yojson/issues/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 thank you for the review, I will address the comments and let you know when it's ready for review again!
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.
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.
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?
~~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.
Oh it seems that I need to rebase, the master has dung-lang > 2.0 so I should get formating for free by rebasing
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.
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.
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.
Thanks for the review @Leonidas-from-XIV I will address the comments and let you know when it's ready for another review round!
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?
Hello @Leonidas-from-XIV it's been a while, I update the MR and I'm asking for another review, please
- I did the TODO about unicode and hex escaping
- I added some tests
- I also squashed all my commits that you already reviewed, some of them were with the wrong email on it, so the last two commits are the new work
Just curious: is the documented TODO tweak the only thing preventing this from being merged?
@vphantom I don't know, I have not had the time to do a proper review in a while to see what works and what is missing. Sorry about that also to @dhilst who put a considerable effort into pushing this onward.
Hello. No problem at all
From my side, I worked on the todo and added tests for the unicode escaping code. I haven't touched this code in a while though, I will try to fix the conflicts soon
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!
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
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.
@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 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!