Unify Json and ppx runtime modules #45
Conversation
There was a problem hiding this comment.
This is looking great @andreypopp !
I fixed the tests (don't ask how 🙈 ) in #46. For now I use the deprecated APIs (also helpful to see how users will have to adapt their code). Once they are removed we could update the tests to use the new APIs because they're quite exhaustive.
As can be seen in the PR, some of the JSON error messages were more detailed before. We should bring these details back somehow, do you think it'll be too hard with the PPX approach?
src/Json.mli
Outdated
| module Of_json : sig | ||
| (** Provides a set of low level combinator primitives to decode | ||
| Js.Json.t data structures A decoder combinator will return the | ||
| decoded value if successful, or raise a [DecodeError of string] if |
There was a problem hiding this comment.
This is not DecodeError anymore.
| (** JSON can be encoded as JSON, trivially. *) | ||
|
|
||
| (** The type of a error which occurs during decoding JSON values. *) | ||
| type of_json_error = Json_error of string | Unexpected_variant of string |
There was a problem hiding this comment.
Unexpected_variant currently always sends "unexpected variant" in the string payload. Should this payload be removed?
There was a problem hiding this comment.
I think I've wanted to include the tag name there.
* main: update setup-ocaml
|
I think #46 is ready:
Re: the last point, maybe we should update the examples code to use the new functions, but we can probably do that once they're removed completely? If you think #46 is good to merge here, feel free to do so. The only things missing would be changelog entry and we should probably test some existing package or project that uses this library to make sure everything works as expected (melange-atdgen-codec-runtime is a good candidate). |
|
@jchavarri thanks! I've merged your PR and then removed |
|
Cool! I started taking a look at upgrading melange atdgen codec runtime, but it seems that library blended |
Ref: #36