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

Add support for objects and classes #173

Merged
merged 3 commits into from
May 6, 2018
Merged

Add support for objects and classes #173

merged 3 commits into from
May 6, 2018

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented May 1, 2018

This is an early preview. Feedback and contributions are welcome

@hhugo
Copy link
Collaborator Author

hhugo commented May 1, 2018

fixes #13

@jberdine
Copy link
Collaborator

jberdine commented May 1, 2018

Excellent, looking great! I couldn't check in great detail, but reading through the implementation I did not see anything concerning or notice any lurking gotchas.

@hhugo hhugo changed the title Add support for objects and classes (DO NOT MERGE) Add support for objects and classes May 5, 2018
@hhugo
Copy link
Collaborator Author

hhugo commented May 5, 2018

Certainly not perfect, not bug free, but I think I'm done the initial support.

Copy link
Collaborator

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

Fantastic! Just a couple minor nits and questions.

src/Ast.ml Outdated
@@ -202,10 +244,14 @@ end = struct

let sub_typ ~ctx typ = check parenze_typ {ctx; ast= typ}

let sub_cty ~ctx typ = {ctx; ast= typ}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: naming a class_type typ is intended?

src/Ast.ml Outdated
let sub_pat ~ctx pat = check parenze_pat {ctx; ast= pat}

let sub_exp ~ctx exp = check parenze_exp {ctx; ast= exp}

let sub_cl ~ctx typ = {ctx; ast= typ}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: naming a class_expr typ is intended?

src/Ast.ml Outdated
@@ -755,6 +1053,15 @@ end = struct
| Some (Some true) -> true
| _ -> false

(** [parenze_cty {ctx; ast}] holds when type [ast] should be parenthesized
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/when type/when class type/ ?

src/Ast.ml Outdated
@@ -989,6 +1325,15 @@ end = struct
else exposed_right_exp Non_apply exp )
| _ -> false

(** [parenze_cl {ctx; ast}] holds when type [ast] should be parenthesized
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/when type/when class expr/ ?

src/Fmt_ast.ml Outdated
(Format.asprintf "%t" (Cmts.preserve (fun () -> fmt_expression c xe)))
in
let fmt_args_grouped e0 a1N =
(* TODO: consider [e0] when groupping *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/groupping/grouping .

This is nontrivial due to the different types of e0 and a1N, right? Any other reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no other reason.

@jberdine jberdine merged commit 53ec15a into ocaml-ppx:master May 6, 2018
@jberdine
Copy link
Collaborator

jberdine commented May 6, 2018

Thanks!

@hhugo hhugo deleted the obj branch May 6, 2018 10:09
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.

3 participants