Add support for OCaml 4.11 #36
Conversation
We are preparing ppx_deriving 4.5.0, a minor release for OCaml 4.11 support. While testing rev-dependencies, CI reported that an error with `Pconst_string`. This PR uses the new `Ppx_deriving.string_of_expression_opt` function to destruct `Pconst_string` in a version-independent way.
|
ppx-deriving PR with CI check for rev-dependencies: ocaml-ppx/ppx_deriving#227 |
We cannot satisfy this constraint just by pinning since the version is not specified in `ppx_deriving.opam`.
|
Also closes #33 |
| match expr with | ||
| | [%expr Bytes.of_string [%e? sub_expr ]] -> sub_expr, false | ||
| | _ -> expr, true in | ||
| match Ppx_deriving.string_of_expression_opt expr with |
There was a problem hiding this comment.
Naive question: we would not need Ppx_deriving.string_of_expression if there was support in metaquot on binding string literals. (We could also get rid of the ifdef above for integer constants, etc.). Is this available?
There was a problem hiding this comment.
This is not available: anti-quotations in patterns are limited to [%e? ... ] (expression), [%t? ... ] (type) and [%p? ... ] (pattern). It could be a nice addition: would you fill an issue? It looks quite boring to implement in ppx_tools, as AFAIK, there is one branch to change per supported OCaml version... Anyway, I think that Ppx_deriving.string_of_expression_opt would still be useful in some other contexts (when we are not in a pattern-matching, or not using metaquot anywhere else).
There was a problem hiding this comment.
There are several versions of ppx_tools around (ppx_tools, ppx_tools_versioned, and I wonder if ppxlib has its own reimplementation), so we would have to be careful if we wanted to implement this globally.
There was a problem hiding this comment.
ppxlib has its own reimplementation (https://github.com/ocaml-ppx/ppxlib/blob/master/metaquot/ppxlib_metaquot.ml)... and I have mine (https://github.com/thierry-martinez/metaquot): I think it is worth proposing your extension at least for some of them (I am quite urged to implement it on mine, for instance!), but yes, the conclusion is that we cannot rely on that to replace Ppx_deriving.string_of_expression_opt.
There was a problem hiding this comment.
Thinking more about it, I am not sure what the interface should be. Ideally we would want [%constr ...], but this is not explicit about the type of constant, so it is not very useful. I guess we could have %lit.string, %lit.int, %lit.float, %lit.int32? (For strings, do we provide provisions to match on the delimiters of quoted string literals?). There is a risk that we would end up reinventing a type of structured constants, encoded through the extension-name syntax.
There was a problem hiding this comment.
Yes, when I read your question, I thought about one extension-name per function in Ast_helper.Const. Even if it is a bit more cryptic, I would follow the convention of one-char names: [%c ... ], [%s ... ], [%i ... ], [%n ... ], [%f ... ], with exceptions for [%i32 ... ] and [%i64 ... ], and perhaps [%integer ... ] for matching an integer literal by its string representation. It is quite ad-hoc, indeed, but not much more than the rest of metaquot...
There was a problem hiding this comment.
I think that cryptic is bad here: we don't want to pollute the extension namespace unnecessarily (maybe there will be a need for another quotation named s in metaquot later, which is used more often or tends to nest more than string literals), and I prefer to make the code reasonably easy to read.
There was a problem hiding this comment.
I'll open an issue in ppx_tools.
There was a problem hiding this comment.
(The issue is ocaml-ppx/ppx_tools#83 )
|
If I understand correctly, in this current state, this PR makes ppx_deriving_protobuf require at least |
|
With the rest of the ecosystem is preparing for 4.11, can this be merged? |
|
Is there anything that prevents this to be merged? |
|
If there was one, I don't remember it. Let's merge! |
|
Kind reminder that this was not released on opam |
|
The package does not work as is. See #39 for follow-up PR TL;DR: needed |
CHANGES: * Add support for OCaml 4.11 (ocaml-ppx/ppx_deriving_protobuf#36) (Thierry Martinez, review by Gabriel Scherer) * Add support for OCaml 4.12 (ocaml-ppx/ppx_deriving_protobuf#39) (Kate Deplaix, review by Gabriel Scherer) * Port to ppx_deriving 5.0 and ppxlib (ocaml-ppx/ppx_deriving_protobuf#39) (Kate Deplaix, review by Gabriel Scherer) * Upgrade the tests from ounit to ounit2 (ocaml-ppx/ppx_deriving_protobuf#39) (Kate Deplaix, review by Gabriel Scherer)
We are preparing ppx_deriving 4.5.0, a minor release for OCaml 4.11 support. While testing rev-dependencies, CI reported that an error with
Pconst_string.This PR uses the new
Ppx_deriving.string_of_expression_optfunction to destructPconst_stringin a version-independent way.