Port to ppxlib #111

rgrinberg · 2019-11-17T07:34:57Z

This is a counterpart to ocaml-ppx/ppx_deriving#206

Port to ppxlib
Remove cppo from tests
Remove ppx_tools.metaquot
Update opam file and constraints

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg · 2019-11-17T07:52:41Z

src/ppx_deriving_yojson.ml

+  let list = gen_def_loc Ast_builder.Default.elist
+  let pstr = gen_def_loc Ast_builder.Default.pstring
+  let plist = gen_def_loc Ast_builder.Default.plist
+  let lam = gen_def_loc Ast_builder.Default.pexp_fun Nolabel None


@thierry-martinez there are some missing functions in your compatibility layer. However, I don't think it matters as much as we should probably just switch to ppxlib's Ast_builder API whenever possible

If the interest is to help plugins migrate to the ppxlib layer as easily as possible (and that sounds good to me), then I would propose to integrate those definitions in Pppx_deriving.Ast_convenience indeed. It looks like this new Ast_convience was implemented by @thierry-martinez in a more low-level way (relying on Ast_helper rather than Ast_builder), your approach may actually also suggest sensible improvements to the implementation of current Ast_convenience definitions.

XVilka · 2020-01-08T05:58:36Z

Seems it can't find ppxlib still.

# (cd _build/default && /home/opam/.opam/4.07/bin/ocamlc.opt -w -40 -w -9 -g -bin-annot -I src/.ppx_deriving_yojson.objs/byte -I /home/opam/.opam/4.07/lib/ocaml-migrate-parsetree -I /home/opam/.opam/4.07/lib/ocaml/compiler-libs -I /home/opam/.opam/4.07/lib/ppx_derivers -I /home/opam/.opam/4.07/lib/ppx_deriving/api -I /home/opam/.opam/4.07/lib/ppx_tools -I /home/opam/.opam/4.07/lib/result -no-al[...]
# File "src/ppx_deriving_yojson.ml", line 1, characters 5-11:
# Error: Unbound module Ppxlib
-       ocamlc src/.ppx_deriving_yojson.objs/byte/ppx_deriving_yojson.{cmi,cmo,cmt} (exit 2)
- (cd _build/default && /home/opam/.opam/4.07/bin/ocamlc.opt -w -40 -w -9 -g -bin-annot -I src/.ppx_deriving_yojson.objs/byte -I /home/opam/.opam/4.07/lib/ocaml-migrate-parsetree -I /home/opam/.opam/4.07/lib/ocaml/compiler-libs -I /home/opam/.opam/4.07/lib/ppx_derivers -I /home/opam/.opam/4.07/lib/ppx_deriving/api -I /home/opam/.opam/4.07/lib/ppx_tools -I /home/opam/.opam/4.07/lib/result -no-alias-deps -o src/.ppx_deriving_yojson.objs/byte/ppx_deriving_yojson.cmo -c -impl src/ppx_deriving_yojson.pp.ml)
- File "src/ppx_deriving_yojson.ml", line 1, characters 5-11:
- Error: Unbound module Ppxlib

XVilka · 2020-05-06T11:17:37Z

Now that ppx_deriving was ported to ppxlib, it's time to revive this.
cc @rgrinberg @gasche @thierry-martinez

Ping me if I can help somehow.

kit-ty-kate · 2020-05-06T15:00:31Z

I agree. Since the next version of ppx_deriving uses ppxlib, this makes the OCaml 4.11 support PR (#118) obsolete and now this PR is required to get the 4.11 support

gasche

I had a look now (... sorry for the delay). The code looks nice overall, see a couple comments. Let me know if you don't have time to handle the stylistic comments yourself, in which case I could try to look at it.

@thierry-martinez : this PR contains proposal for extensions to your Ast_convenience reimplementation, which it would be nice to integrate upstream in Ppx_deriving. It also uses a different implementation style (using Ppxlib's Ast_builder) which could be an improvement to the current implementation. Could you let me know whether you (agree this would be a sensible idea and) would be available to integrate some of this in ppx_deriving in the coming week(s), or whether I should try to look at it?

We can still merge the current PR as-is if we want (maybe after some minor stylistic comments have been handled), and simplify the code later if Ppx_deriving.Ast_convenience gains the new combinators; we do not need a dependency on an upstream Ppx_deriving change.

gasche · 2020-05-09T15:12:19Z

src/ppx_deriving_yojson.ml

+
+open Ast_builder_default_loc
+
+let warning_39 () =


I think it could be clearer if this was named disable_warning_39.

Done in 84a0c33

gasche · 2020-05-09T15:16:34Z

src/ppx_deriving_yojson.ml

+  let list = gen_def_loc Ast_builder.Default.elist
+  let pstr = gen_def_loc Ast_builder.Default.pstring
+  let plist = gen_def_loc Ast_builder.Default.plist
+  let lam = gen_def_loc Ast_builder.Default.pexp_fun Nolabel None


If the interest is to help plugins migrate to the ppxlib layer as easily as possible (and that sounds good to me), then I would propose to integrate those definitions in Pppx_deriving.Ast_convenience indeed. It looks like this new Ast_convience was implemented by @thierry-martinez in a more low-level way (relying on Ast_helper rather than Ast_builder), your approach may actually also suggest sensible improvements to the implementation of current Ast_convenience definitions.

gasche · 2020-05-09T15:18:40Z

src/ppx_deriving_yojson.ml

@@ -492,7 +468,7 @@ let ser_str_of_type_ext ~options ~path:_ ({ ptyext_path = { loc }} as type_ext)
      Ppx_deriving.mangle_lid
        (`PrefixSuffix ("M", "to_yojson")) type_ext.ptyext_path.txt
    in
-    String.concat "." (Longident.flatten mod_lid)
+    String.concat "." (Longident.flatten_exn mod_lid)


The whole line could be written Longident.name mod_lid instead.

Done in 5a26808

gasche · 2020-05-09T15:18:53Z

src/ppx_deriving_yojson.ml

@@ -704,7 +680,7 @@ let desu_str_of_type_ext ~options ~path ({ ptyext_path = { loc } } as type_ext)
      Ppx_deriving.mangle_lid
        (`PrefixSuffix ("M", "of_yojson")) type_ext.ptyext_path.txt
    in
-    String.concat "." (Longident.flatten mod_lid)
+    String.concat "." (Longident.flatten_exn mod_lid)


(Here again Longident.name would be nice.)

Done in 5a26808

XVilka · 2020-05-09T15:31:26Z

I would suggest waiting until also ppx_deriving_protobuf is ported, then we can see if some helpers need to be moved to ppx_deriving itself, and removed from here and ppx_deriving_protobuf.

XVilka · 2020-05-09T15:32:46Z

@gasche also please look at the #118 - it superseded this PR

Suggested by Gabriel Scherer: ocaml-ppx#111 (comment)

Suggested by Gabriel Scherer: ocaml-ppx#111 (comment) and ocaml-ppx#111 (comment)

thierry-martinez · 2020-05-21T14:16:18Z

I opened a new PR #121 to include @gasche's suggestions and @kit-ty-kate's #118 work for OCaml 4.11 as well.

Port to ppxlib and OCaml 4.11 (continuation of #111 and #118)

gasche · 2020-05-24T18:10:59Z

This is superseded by #121 which I just merged. Thanks a lot, and sorry for the lack of reactivity.

rgrinberg added 5 commits November 17, 2019 16:30

Port to ppxlib

845a643

Remove cppo processing from ppx_deriving

2d25f68

Signed-off-by: Rudi Grinberg <[email protected]>

Switch to ppxlib's metaquot

5cef538

Signed-off-by: Rudi Grinberg <[email protected]>

Remove ppx_tools and ppxfind in favor of ppxlib

ba531bd

Signed-off-by: Rudi Grinberg <[email protected]>

Remove cppo entirely

2ca766f

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg mentioned this pull request Nov 17, 2019

Remove 4.03-related ifdefs #95

Closed

rgrinberg commented Nov 17, 2019

View reviewed changes

kit-ty-kate mentioned this pull request May 6, 2020

OCaml 4.11 support #118

Closed

gasche reviewed May 9, 2020

View reviewed changes

thierry-martinez mentioned this pull request May 21, 2020

Port to ppxlib and OCaml 4.11 (continuation of #111 and #118) #121

Merged

thierry-martinez added a commit to thierry-martinez/ppx_deriving_yojson that referenced this pull request May 21, 2020

Rename warning_39 to disable_warning_39

84a0c33

Suggested by Gabriel Scherer: ocaml-ppx#111 (comment)

thierry-martinez added a commit to thierry-martinez/ppx_deriving_yojson that referenced this pull request May 21, 2020

Use Longident.name

5a26808

Suggested by Gabriel Scherer: ocaml-ppx#111 (comment) and ocaml-ppx#111 (comment)

gasche added a commit that referenced this pull request May 24, 2020

Merge pull request #121 from thierry-martinez/411

d067114

Port to ppxlib and OCaml 4.11 (continuation of #111 and #118)

gasche closed this May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to ppxlib #111

Port to ppxlib #111

rgrinberg commented Nov 17, 2019 •

edited

Loading

rgrinberg Nov 17, 2019

gasche May 9, 2020

XVilka commented Jan 8, 2020

XVilka commented May 6, 2020 •

edited

Loading

kit-ty-kate commented May 6, 2020

gasche left a comment

gasche May 9, 2020

thierry-martinez May 21, 2020

gasche May 9, 2020

gasche May 9, 2020

thierry-martinez May 21, 2020

gasche May 9, 2020

thierry-martinez May 21, 2020

XVilka commented May 9, 2020

XVilka commented May 9, 2020

thierry-martinez commented May 21, 2020

gasche commented May 24, 2020

Port to ppxlib #111

Port to ppxlib #111

Conversation

rgrinberg commented Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVilka commented Jan 8, 2020

XVilka commented May 6, 2020 • edited Loading

kit-ty-kate commented May 6, 2020

gasche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVilka commented May 9, 2020

XVilka commented May 9, 2020

thierry-martinez commented May 21, 2020

gasche commented May 24, 2020

rgrinberg commented Nov 17, 2019 •

edited

Loading

XVilka commented May 6, 2020 •

edited

Loading