Skip to content

Conversation

@gasche
Copy link
Contributor

@gasche gasche commented Jan 16, 2021

Currently Ezjsonm does not provide users with the ability to properly
report errors. In particular, the error location inside the JSON
source is provided by Jsonm and thrown away by Ezjsonm.

In ocaml-mustache I had to remove the Ezjsonm usage and turn it into Jsonm directly to provide sensible error messages: rgrinberg/ocaml-mustache#63.

This PR provides *_result variants of reading functions
that (instead of failing in badly-documented ways) return an
informative error type in the case of reading failure, along
with simple functions to provide basic error messages to users.

Note: this PR uses the "result" type from the standard library, and as
a consequence it bumps the required OCaml version from 4.04 to
4.08. If this is judged too strong, we could use a compatibility
"result" module instead.

Finally: I would like to write tests for this PR, but I am not sure what would be the right approach. Any recommendation/preference?

@dinosaure
Copy link
Member

We prefer an open polymorphic variant for errors (eg. [> error ]) to be able to compose it with others errors, can you update your PR? Otherwise, seems good. For tests, we use alcotest. About #63, I will try to find a solution to merge both worlds 👍.

Currently Ezjsonm does not provide users with the ability to properly
report errors. In particular, the error location inside the JSON
source is provided by Jsonm and thrown away by Ezjsonm.

This PR provides *_result variants of reading functions
that (instead of failing in badly-documented ways) return an
informative error type in the case of reading failure, along
with simple functions to provide basic error messages to users.

Note: this PR uses the "result" type from the standard library, and as
a consequence it bumps the required OCaml version from 4.04 to
4.08. If this is judged too strong, we could use a compatibility
"result" module instead.
@gasche
Copy link
Contributor Author

gasche commented Jan 19, 2021

Thanks for the feedback. I changed the PR to use open variants as suggested.

@gasche
Copy link
Contributor Author

gasche commented Feb 17, 2021

I just pushed some tests for from_string_result. I consider this PR complete on my end.

@dinosaure
Copy link
Member

I'm currently on a merge of your PR and #42 👍.

@gasche
Copy link
Contributor Author

gasche commented Feb 17, 2021

Regarding #42, as mentioned I think that the proposed implementation is more complex than necessary. I am thinking of doing an implementation directly for Jsonm upstream, but I think that the version I suggested in the comments would basically be fine for Ezjsonm. I don't mind rebasing this PR if you merge some version of #42 before it.

@dinosaure dinosaure merged commit 0c58782 into mirage:master Feb 24, 2021
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Nov 12, 2021
CHANGES:

* Provide `*_result` variants of reading functions with informative errors
  (@gasche, @dinosaure, mirage/ezjsonm#43)
* Improve support of `js_of_ocaml` and be able to parse huge JSON values
  (@hhugo, @dinosaure, @avsm, @smondet, @gasche)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants