ocaml-ppx / ppxlib

Base library and tools for ppx rewriters
MIT License
244 stars 94 forks source link

Ppx_deriving utility functions #317

Open sim642 opened 2 years ago

sim642 commented 2 years ago

Even though ppxlib is recommended for implementing new ppx derivers, ppxlib itself doesn't have a bunch of utility functions that are provided by ppx-deriving.api: https://github.com/ocaml-ppx/ppx_deriving/blob/master/src/api/ppx_deriving.cppo.mli. In particular:

I would say that most nontrivial ppx derivers need that functionality anyway. Currently there's two options:

  1. Just depend on ppx-deriving.api in addition to ppxlib. This is done by, e.g., ppx_deriving_yojson. This is not ideal, since the API package doesn't just contain the utilities, but also ppx-deriving's own registration system, which is replaced by ppxlib. Moreover, it prevents deprecation of the API in favor of ppxlib: https://github.com/ocaml-ppx/ppx_deriving/pull/250.
  2. Copy the utility functions' implementations to own ppx deriver project. This is done by, e.g., ppx_show. This avoids the dependency but involves copying code, which isn't ideal either. Moreover, fixing or optimizing those utilities is much easier in a central place (https://github.com/ocaml-ppx/ppx_deriving/pull/252).
pitag-ha commented 1 year ago

Thanks @sim642 for opening and working on this!

Are you also working or would like to work on the missing one, i.e. polymorphism handling? I was just ask by someone what could be a good issue to work on for someone new working on ppxlib and this could be a fit - unless you wanted to implement it yourself of course.

sim642 commented 1 year ago

Feel free to let them do it! I won't rush to it then.

Once we fix the submodule name for all these utilities in #370, then it should be very straightforward to bring any remaining ones over from ppx_deriving. If I recall correctly, Ppx_deriving contains a bunch of utility functions that don't belong to any of the categories I initially listed, but might be worth bringing over nevertheless.