of_yojson_exn breaks code that defines of/to_yojson by hand

[emillon]

I'm commenting a bit late on the issue, but adding of_yojson_exn (see #68) breaks all code that defines of_yojson by hand ([@@deriving of_yojson] in the mli, let of_yojson in the ml).

This means that to support this, users will have to copy/paste let of_yojson_exn json = match of_yojson json with Ok x -> x | Error e -> failwith e or similar. It would be nice if ppx_deriving_yojson could handle these cases automatically, but it's unfortunately not possible.

I suggest that we revert this part before the next release and advise users to use a combinator like CCResult.get_exn or RResult.R.get_ok. An alternative is to make this feature opt-in: for example if [@@deriving yojson { exn = true }] is present, generate the of_yojson_exn function in the .ml, and add the corresponding val in the .mli.

Thanks!

[whitequark]

cc @stevebleazard

I think the right path here is to make the feature opt-in, not revert. Opinions @ocaml-ppx/deriving?

[emillon]

To make this feature play nice with existing code, we could also always use of_yojson from dependencies instead of calling of_yojson_exn recursively. That way, it can use dependencies that do not have exn = true (that's similar to how show uses pp, so defining pp in libraries is enough).

[whitequark]

I agree that this is better.