search

ocaml-ppx / ppx_deriving

Type-driven code generation for OCaml
MIT License
466 stars 89 forks source link

Port standard plugins to ppxlib registration and attributes #263

Closed sim642 closed 8 months ago

sim642 commented 2 years ago

This realizes part of #250:

  1. All the standard deriving plugins defined here are switched to register themselves directly with ppxlib and declare their attributes directly with ppxlib.
  2. Deprecates only parts of ppx_deriving API, namely ppx_deriving deriver registration and attribute support. All the other utility functions remain undeprecated since many are still missing from ppxlib (https://github.com/ocaml-ppx/ppxlib/issues/317).
  3. Delegates ppx_deriving quoter to ppxlib quoter.

Notably, this makes [@@deriving_inline ...] work on these standard derivers.

TODO

pitag-ha commented 2 years ago

Hey @sim642, even though I'm still not finding the time to review (as we've mentioned already, finding time to work on PPX-related things is rare), I at least already wanted to take the time to thank you. Thanks for the PR! I personally would have preferred to write a new bunch of standard derivers to have more freedom to improve things (as started here, but never finished/gotten far). However, I appreciate the work and it's definitely good to have these standard derivers written in ppxlib. It might still take a bit, but when we some have time for PPX we'll review this - if not me, then @panglesd. Feel free to ping us once in a while.

sim642 commented 1 year ago

@pitag-ha @panglesd ping!

panglesd commented 1 year ago

Very sorry about the delay. I'll put that on the top of my todo list. (Although next week I'm on holidays).

sim642 commented 1 year ago

Regarding the deprecations, it would be fine to also just exclude them from this PR. If I remember correctly, the original inspiration was #250, which proposes to deprecate the entire API. However, given warnings-as-errors in many projects, this might cause unnecessary breakage of derivers still built on ppx_deriving. Having everything ported would be nice, but these things don't have to be coupled.

I might've also used the deprecation warnings as a guide to find any usages to port within these standard plugins.

Is Arg deprecated in favour of Ast_pattern?

It's not even marked deprecated here right now, but I think I wanted to make as big step towards fully using ppxlib as possible, hence the switch to Ast_pattern which should replace it. In a way, this PR serves as an exercise in porting such derivers and revealing places where ppx_deriving's old API is more convenient than ppxlib's new one.

panglesd commented 1 year ago

About the inclusion of deprecation of the API in this PR: I don't have a strong opinion at all...

If there is a clear indication of what should be used instead, and it's easy to fix (for instance, the mangle function, but not the Arg module), then I think it is OK: when built by opam, warnings are not errors, and when built locally, it is easy to follow the instructions to fix the deprecations warning.

But, I agree that it is not completely necessary, and might upset some people. Maybe, a mention in the ppx_deriving documentation is enough.

sim642 commented 1 year ago

I've just excluded all the deprecations from this PR right now to avoid blocking this by those additional discussions and decisions.

panglesd commented 1 year ago

Looks good to merge to me. I opened the issues you found with the API. I guess you could wait for them to be implemented (I consider having them as good first issues for the ongoing Outreachy round, but when this is finished, it shouldn't take long to implement) to simplify the code. Or, merge now and simplify later.

sim642 commented 1 year ago

Hey! This has been sitting around for quite a while now. It'd be great to get this merged and released as well. It's been a long while since ppx_deriving has seen a release (even #252 hasn't made it).

gasche commented 1 year ago

I'm one of the few maintainers of ppx_deriving, mostly inactive. I can give some time next week to look at this again and move towards merging, but I prefer to let other people take care of releases. Do we have a volunteer to cut out a new release? (I'm happy to give commit rights to enable this.)

kit-ty-kate commented 1 year ago

@pitag-ha @panglesd do you want the merge rights by any chance?

panglesd commented 1 year ago

Hello ! Sorry I'm off during one month, I will assess whether I accept this responsibility when I come back!

pitag-ha commented 1 year ago

@pitag-ha @panglesd do you want the merge rights by any chance?

I don't :) Let's see if @panglesd happens to want them when he's back. Btw, what's the current maintenance status of ppx_deriving? And what's the current general status of ppx_deriving? From what I remember, ppx_deriving is still used in a few contexts:

  1. A few derivers are still registered with ppx_deriving. IIRC, ppx_deriving just forwards the registration to ppxlib in that case. a) The ppx_deriving.std plug-ins are still registered with ppx_deriving (changes with this PR). b) A few other derivers are still registered with ppx_deriving (e.g. visitors?).
  2. The ppx_deriving driver is still used in a few cases. a) utop, and possibly other toplevels as well, use the ppx_deriving driver for derivers rather than the ppxlib driver. b) Is there any build system that uses ppx_deriving? (dune doesn't, but I don't know about other build systems or people who write their custom build Makefiles)

Is that right?

I can give some time next week to look at this again and move towards merging

Thank you, @gasche! If that would help, I could answer questions about Ppxlib if they come up.

panglesd commented 11 months ago

Sorry for the long delay, I'm back.

@NathanReb since you have joined as a ppxlib maintainer, there is a new variable in the equation! Would you be interested in any activity involving this repository?

If not, I will be happy to take care of cutting a release that includes this change. That being said, I can't commit to be an active maintainer in the long run!

NathanReb commented 10 months ago

I'd be happy to take part in smoothing things out here yes! Will need to catch up a bit but merging and releasing this work seems important for the ppx ecosystem so I'll gladly to take care of it!

kit-ty-kate commented 10 months ago

@gasche could you add @NathanReb ? I thought i had the rights to do that but turns out i only have access to some of the settings and not this one (github maintainer vs. admin)

gasche commented 10 months ago

Done, and I made you @kit-ty-kate an Admin. Thanks!

NathanReb commented 8 months ago

@sim642 sorry for the long wait. I will work on getting this merged and will start reviewing it.

Are you still willing to work on this if there are changes to be made?

sim642 commented 8 months ago

Are you still willing to work on this if there are changes to be made?

Yes, that shouldn't be an issue.

NathanReb commented 8 months ago

Sorry again for the long delays and thanks for taking the time to get back to this @sim642. Really appreciate it!

Let's merge this!