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

Ppx deriving #364

Merged
merged 13 commits into from Dec 4, 2015
Merged

Ppx deriving #364

merged 13 commits into from Dec 4, 2015

Conversation

@vasilisp
Copy link
Contributor

@vasilisp vasilisp commented Oct 5, 2015

This is a PPX version of our JSON deriving plug-in.

The generated code is somewhat different from the Camlp4 version. We follow the conventions of ppx_deriving, as opposed to the functorial style of (Camlp4) deriving.

The serialization format is the same, i.e., in principle you can dump x with Camlp4, parse the result with PPX, and get back x. Any divergence is a bug to be fixed.

I provide a few tests that demonstrate the syntax as a Gist.

@Drup
Copy link
Member

@Drup Drup commented Oct 5, 2015

Please add your tests directly in the repo.
I'll review later.

cc @whitequark it may interest you :)

List.map f l
in
(* CHECKME: what is the closed_flag for? *)
Ast_helper.Pat.record l Asttypes.Closed

This comment has been minimized.

@whitequark

whitequark Oct 5, 2015

This is to differentiate between { x } and { x; _ }. The former has an exhaustiveness warning, turned off by default.

@whitequark
Copy link

@whitequark whitequark commented Oct 5, 2015

LGTM, but camlp4 is not writhing enough in fiery hell

@vasilisp vasilisp force-pushed the ppx_deriving branch from 5883be1 to eacbbf4 Oct 22, 2015
@vasilisp vasilisp force-pushed the ppx_deriving branch from eacbbf4 to 060954a Nov 9, 2015
@vasilisp vasilisp force-pushed the ppx_deriving branch 4 times, most recently from 50ae0e2 to effcc44 Nov 23, 2015
@hhugo
Copy link
Member

@hhugo hhugo commented Dec 3, 2015

Any update on this ?

@vasilisp vasilisp force-pushed the ppx_deriving branch from ffa1ea0 to d323e5a Dec 4, 2015
@vasilisp
Copy link
Contributor Author

@vasilisp vasilisp commented Dec 4, 2015

@hhugo, the latest commits fix omissions discovered while testing in conjunction with Eliom PPX.

I guess nobody has felt the urge to merge this, but it should be merge-able.

Drup added a commit that referenced this pull request Dec 4, 2015
Ppx deriving
@Drup Drup merged commit efea951 into master Dec 4, 2015
1 check failed
1 check failed
@ocsigen-buildbot
default Merged build finished.
Details
@vasilisp vasilisp deleted the ppx_deriving branch Dec 4, 2015
@hhugo
Copy link
Member

@hhugo hhugo commented Dec 13, 2015

Can someone fix the code to remove the following warnings

ocamlfind ocamlc     -w +A-4-7-9-37-38-41-44-45 -w -40-42-48-27 -package ppx_deriving,ppx_tools,ppx_tools.metaquot -c ppx/ppx_deriving_json.ml
File "ppx/ppx_deriving_json.ml", line 108, characters 2-811:
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a value that is not matched:
Rinherit
  {ptyp_desc=(Ptyp_any|Ptyp_var _|Ptyp_arrow (_, _, _)|Ptyp_tuple _|
             Ptyp_object (_, _)|Ptyp_class (_, _)|Ptyp_alias (_, _)|
             Ptyp_variant (_, _, _)|Ptyp_poly (_, _)|Ptyp_package _|
             Ptyp_extension _)}
File "ppx/ppx_deriving_json.ml", line 245, characters 33-919:
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a value that is not matched:
Rinherit
  {ptyp_desc=(Ptyp_any|Ptyp_var _|Ptyp_arrow (_, _, _)|Ptyp_tuple _|
             Ptyp_object (_, _)|Ptyp_class (_, _)|Ptyp_alias (_, _)|
             Ptyp_variant (_, _, _)|Ptyp_poly (_, _)|Ptyp_package _|
             Ptyp_extension _)}
File "ppx/ppx_deriving_json.ml", line 34, characters 15-43:
Warning 26: unused variable lid.
File "ppx/ppx_deriving_json.ml", line 127, characters 27-54:
Warning 26: unused variable c.
File "ppx/ppx_deriving_json.ml", line 222, characters 12-13:
Warning 26: unused variable f.
File "ppx/ppx_deriving_json.ml", line 258, characters 13-55:
Warning 26: unused variable y'.
@vasilisp
Copy link
Contributor Author

@vasilisp vasilisp commented Dec 15, 2015

I will fix this ASAP.

@hhugo hhugo mentioned this pull request Mar 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants