-
Notifications
You must be signed in to change notification settings - Fork 90
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
Port to ppxlib 0.16 / ocaml-migrate-parsetree 2.0.0 #231
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).
Tentative lightning update for OCaml 4.11.0 before introducing ppxlib
…dencies This should implement @gasche suggestion: ocaml-ppx#183 We should do that for `master` as well (I expect there will be more failures there!) but I begin here.
This reverts commit d874295.
Suggested by Gabriel Scherer.
I think that CI results on recent versions are often those that are the most interesting, so it is better to compute them first.
It is the default. Here is a pull-request to allow reverse-dependencies to be tested. ocaml/ocaml-ci-scripts#338
(Let me explicitate some context that is shared with @thierry-martinez, but may help other people finding the PR, or ourselves in the future.) In the previous implementation, we were careful to use Ppxlib but without using the OCaml AST version it encourages, rather using the "current" version whenever possible -- using ocaml-migrate-parsetree to transform from one to another. This choice was made to avoid a difficulty coming from Ppxlib's non-versioned design, where users are "stuck" with one version of OCaml that may be lagging a few releases behind the current OCaml release. In recent months, ppxlib maintainers have taken step to reduce the lag, and announced their intention to follow closely new OCaml releases. This means that we will have to update more often (if we want to follow recent ppxlib releases), but our users will not pay the cost of the "lag", even if we use Ppxlib's AST directly instead of the "current" AST. This PR applies this design change. This simplifies the implementation quite a bit, which is excellent news. Another change in this PR is a removal of the driver-registration logic, which was done using ocaml-migrate-parsetree parser facilities. The ppxlib maintainers (who also maintain ocaml-migrate-parsetree) have decided to consolidate all driver logic in ppxlib, so we cannot do a non-ppxlib registration anymore, and we can again simplify the implementation by removing this feature. In terms of code, this PR thus has very nice properties. I'm not completely sure that the change preserves compatibility for plugin authors, or whether we will have to upgrade plugins again. |
Thanks, @gasche, for the recap. I tried to add |
Superseded by #237 |
This PR is an attempt to port
ppx_deriving
toppxlib 0.16
.