Port standard plugins to ppxlib registration and attributes
This realizes part of #250:
- All the standard deriving plugins defined here are switched to register themselves directly with ppxlib and declare their attributes directly with ppxlib.
- ~~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).~~
- Delegates ppx_deriving quoter to ppxlib quoter.
Notably, this makes [@@deriving_inline ...] work on these standard derivers.
TODO
- [x] Re-add
derive.*extensions support? - [ ] Fix missing optional derivers support? Separate issue really: #247.
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.
@pitag-ha @panglesd ping!
Very sorry about the delay. I'll put that on the top of my todo list. (Although next week I'm on holidays).
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
Argdeprecated in favour ofAst_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.
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.
I've just excluded all the deprecations from this PR right now to avoid blocking this by those additional discussions and decisions.
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.
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).
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.)
@pitag-ha @panglesd do you want the merge rights by any chance?
Hello ! Sorry I'm off during one month, I will assess whether I accept this responsibility when I come back!
@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:
- A few derivers are still registered with
ppx_deriving. IIRC,ppx_derivingjust forwards the registration toppxlibin that case. a) Theppx_deriving.stdplug-ins are still registered withppx_deriving(changes with this PR). b) A few other derivers are still registered withppx_deriving(e.g.visitors?). - The
ppx_derivingdriver is still used in a few cases. a)utop, and possibly other toplevels as well, use theppx_derivingdriver for derivers rather than theppxlibdriver. b) Is there any build system that usesppx_deriving? (dunedoesn'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.
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!
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!
@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)
Done, and I made you @kit-ty-kate an Admin. Thanks!
@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?
Are you still willing to work on this if there are changes to be made?
Yes, that shouldn't be an issue.
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!