-
Notifications
You must be signed in to change notification settings - Fork 94
Upgrade to ppxlib 0.16.0 #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
To accomodate `Pconst_string` for OCaml 4.11, I suggest the following changes: - constructing a `Pconst_string` can be done with `Ast_helper.Const.string` - for destructing a `Pconst_string`, this pull-request introduces `string_of_constant_opt` and `string_of_expression_opt` in `ppx_deriving.runtime`. I think indeed that both functions can be useful for rev-dependencies to write OCaml version agnostic code. I add a compatibility implementation for `Option` module in `ppx_deriving.runtime` to be able to write more idiomatic code, and again, I think that it can be useful for rev-dependencies (as we already do for `Result`).
Suggested by @gasche.
`Result.result` and `result` seems to be incompatible, at least in 4.04.2: the "include (module type of Pervasives with ...)" above forgets some equalities and should be replaced in another PR.
Suggested by Gabriel Scherer. ocaml-ppx#222 (comment) There should be: - no space between the macro name Pconst_string_patt and the arguments (because the arguments are for the macro), but - a space between the macro name Pconst_string and the arguments (because the arguments are not for the macro but for the underlying constructor).
CI is green. |
CHANGES: * Update to ppxlib 0.16.0 ocaml-ppx/ppx_deriving#237 (Thierry Martinez, Gabriel Scherer, Kate Deplaix)
Sounds like I was a little to overexcited for this. I should've done more testing... @NathanReb any idea what's wrong in this PR? The change gave me a slew of revdeps failures here: https://ci.ocaml.org/ocaml/opam-repository/pr/17539?test=2%20Revdeps Most of them use ocamlfind instead of dune (that's why I couldn't detect this in the tests) and fail in a similar manner:
There is also a more annoying one here:
|
(module Migrate_parsetree.OCaml_current) | ||
(fun _ _ -> mapper) | ||
Ppxlib.Driver.register_transformation "ppx_deriving" | ||
~preprocess_impl:map_structure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preprocess_intf
and preprocess_impl
are special fields used to define a transformation that must run before anything else and as it turns out you can only use one such ppx at any given time, this is why it's incompatible with ppx_optcomp
. I'm guessing this is required because ppxlib otherwise throws warnings about uninterpreted deriving plugins right?
I'd have to take a deeper look but to be honest I wouldn't put too much effort into this, the plan is to migrate If users feel like ppxlib lacks some important ppx_deriving features, I'd be happy to hear about them. We already started importing features to ppxlib to make the migration easier. |
Who is going to work on migrating all ppx_deriving plugins to pure ppxlib plugins? The work here (which is a continuation of the work by @thierry-martinez) is precisely trying to start such a migration effort, and yet you seem to suggest that it's not the right way to proceed. If someone is volunteering to implement the remaining ppx_deriving plugins in ppxlib directly, I'm happy to see it done, but until it happens we would welcome your help ensuring that the migration to ppxlib is smooth enough (as was suggested in the past). |
In the meantime we can start by adding a conflict with I'll look into the other failure. @gasche I'm not saying it's not the right way to proceed, just that it comes with some limitations. I'm not sure the cost of overcoming those in ppxlib is smaller than the cost of migrating the few widely used plugins. I'm also not sure it's worth the maintenance cost it will have on ppxlib in the long run. A good intermediate step would be to try to partially rewrite ppx_deriving so that it registers its plugins using |
@@ -1,101 +0,0 @@ | |||
open Ppxlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think clearing out this file is responsible for breaking compat with ocamlbuild!
CHANGES: * Update to ppxlib 0.20.0 ocaml-ppx/ppx_deriving#237 ocaml-ppx/ppx_deriving#239 ocaml-ppx/ppx_deriving#243 ocaml-ppx/ppx_deriving#245 (Kate Deplaix, Jérémie Dimino, Thierry Martinez, Gabriel Scherer) * Upgrade testsuite from ounit to ounit2 ocaml-ppx/ppx_deriving#241 (Kate Deplaix) * (almost) use the set of standard flags from dune ocaml-ppx/ppx_deriving#246 (Kate Deplaix)
CHANGES: * Update to ppxlib 0.20.0 ocaml-ppx/ppx_deriving#237 ocaml-ppx/ppx_deriving#239 ocaml-ppx/ppx_deriving#243 ocaml-ppx/ppx_deriving#245 (Kate Deplaix, Jérémie Dimino, Thierry Martinez, Gabriel Scherer) * Upgrade testsuite from ounit to ounit2 ocaml-ppx/ppx_deriving#241 (Kate Deplaix) * (almost) use the set of standard flags from dune ocaml-ppx/ppx_deriving#246 (Kate Deplaix)
CHANGES: * Update to ppxlib 0.20.0 ocaml-ppx/ppx_deriving#237 ocaml-ppx/ppx_deriving#239 ocaml-ppx/ppx_deriving#243 ocaml-ppx/ppx_deriving#245 (Kate Deplaix, Jérémie Dimino, Thierry Martinez, Gabriel Scherer) * Upgrade testsuite from ounit to ounit2 ocaml-ppx/ppx_deriving#241 (Kate Deplaix) * (almost) use the set of standard flags from dune ocaml-ppx/ppx_deriving#246 (Kate Deplaix)
CHANGES: * Update to ppxlib 0.20.0 ocaml-ppx/ppx_deriving#237 ocaml-ppx/ppx_deriving#239 ocaml-ppx/ppx_deriving#243 ocaml-ppx/ppx_deriving#245 (Kate Deplaix, Jérémie Dimino, Thierry Martinez, Gabriel Scherer) * Upgrade testsuite from ounit to ounit2 ocaml-ppx/ppx_deriving#241 (Kate Deplaix) * (almost) use the set of standard flags from dune ocaml-ppx/ppx_deriving#246 (Kate Deplaix)
CHANGES: * Update to ppxlib 0.20.0 ocaml-ppx/ppx_deriving#237 ocaml-ppx/ppx_deriving#239 ocaml-ppx/ppx_deriving#243 ocaml-ppx/ppx_deriving#245 (Kate Deplaix, Jérémie Dimino, Thierry Martinez, Gabriel Scherer) * Upgrade testsuite from ounit to ounit2 ocaml-ppx/ppx_deriving#241 (Kate Deplaix) * (almost) use the set of standard flags from dune ocaml-ppx/ppx_deriving#246 (Kate Deplaix)
This PR simply rebases #231 on top of master.
Here is the diff with the latest commit of #231 (d51cdbd):
I'm not sure where I got the
(** {2 Coercions} *)
bit back from but it seems to have been deleted on master, however it seems to make more sense to have it so I readded it.