ppx_deriving_yojson icon indicating copy to clipboard operation
ppx_deriving_yojson copied to clipboard

Published 20 hours ago • verified publisher icon ocaml-ppx

Remove constraint on Yojson 2

Open Leonidas-from-XIV opened this issue 3 years ago • 2 comments

I tried running the tests with Yojson 2 and they seem to… just work. So this constraint seems to be overly cautionous.

Leonidas-from-XIV avatar Jun 13 '22 09:06 Leonidas-from-XIV

Not quite, the old Yojson.Safe.json type is still referenced is some places:

https://github.com/ocaml-ppx/ppx_deriving_yojson/blob/64c3af970e4e0d0f21cdfc5187968aa1d35fc21e/src/ppx_deriving_yojson.ml#L122

It might be better to remove them and require yojson >= 2.0, but maybe since they’re in the left part of the pattern it’s fine to leave. I don’t know.

kit-ty-kate avatar Jun 23 '22 10:06 kit-ty-kate

Yes, it is there but it seems like it doesn't hurt, somehow what it compiles to does not cause issues. Probably because packages that use Yojson.Safe.json in their types for derivation can't use Yojson 2.0 anyway and have to have the restriction applied there.

I can also remove that match pattern, since Yojson.Safe.t was added in 1.6 and ppx_deriving_yojson requires 1.6 already.

Leonidas-from-XIV avatar Jun 23 '22 10:06 Leonidas-from-XIV

Seems to have been done by https://github.com/ocaml-ppx/ppx_deriving_yojson/commit/69671f7653c210ac926c498976af3f30a5948f9c

anmonteiro avatar Aug 29 '22 16:08 anmonteiro

Oops, indeed. Sorry i missed your PR

kit-ty-kate avatar Aug 29 '22 16:08 kit-ty-kate