Skip to content
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

Make ppx deriving compatible with preprocessor type ppxlib extensions #245

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

kit-ty-kate
Copy link
Collaborator

This is an attempt at fixing the last issue of ppx_deriving with ocaml-migrate-parsetree 2.0

The problem was that making ppx_deriving a "preprocesor type ppxlib extension", made it imcompatible with other "preprocessor type ppxlib extensions" such as optcomp and triggered errors in a handful of packages in opam-repository.

From what I understand this was done to enforce an order between ppxs, and when mentioning this issue the ppxlib devs told me to use the Ppxlib.Deriving API. However, I'm no expert in either ppxlib nor ppx_deriving and didn't manage to make it work in a reasonable time.

Here I'm using the cheap method of simply stop caring about orders, I've ran a test on every opam packages for OCaml 4.11 with this change and no issue was found. So, to me, this seems safe to do so, since from what I understand, the previous code using OMP1 did not care about order either.

What do you think? @thierry-martinez @gasche

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems reasonable to me, and the new code makes more sense to me. You could probably find a combination of ppx rewriters where this is an issue, but you seem to have done enough testing here.

@kit-ty-kate
Copy link
Collaborator Author

Thanks a lot for the review! Merging and tagging the release now 🎉

@kit-ty-kate kit-ty-kate merged commit f508891 into ocaml-ppx:master Nov 25, 2020
@kit-ty-kate kit-ty-kate deleted the ppxlib-no-preprocessor branch November 25, 2020 20:01
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Nov 25, 2020
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)
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Nov 25, 2020
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)
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Nov 25, 2020
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)
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Nov 25, 2020
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)
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Nov 25, 2020
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant