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

Pretty print extensions #1703

Merged
merged 10 commits into from
Dec 26, 2017
Merged

Pretty print extensions #1703

merged 10 commits into from
Dec 26, 2017

Conversation

let-def
Copy link
Contributor

@let-def let-def commented Dec 13, 2017

With current version, Reason accepts:

%lwt
let x = Lwt.return(5);

But pretty print it as:

[%%lwt let x = Lwt.return(5)];

This PR implements pretty-printing for extension sugars (in structures and in expressions).

@hcarty
Copy link
Contributor

hcarty commented Dec 13, 2017

Very cool! Does this work for match, if and try? Those are all used/supported by Lwt and match and if are supported by ppx_let.

@let-def
Copy link
Contributor Author

let-def commented Dec 14, 2017

Yes, these are supported, I added them to the test.

However, the sugar triggers only in "sequences" (between {/}, so either with a ...; ... or let ... = ...; ...).

So when there is only one expression, it falls back to [%lwt ...].
Compare:

@let-def
Copy link
Contributor Author

let-def commented Dec 14, 2017

So the question remains what should be the printing for a single expression:

let x = {
  %extend
  switch None {
    | Some(x) => assert(false)
    | None => ()
  };
};
let x = [%extend
  switch None {
    | Some(x) => assert(false)
    | None => ()
  };
];
let x = (%extend
  switch None {
    | Some(x) => assert(false)
    | None => ()
  };
);
let x = %extend
  switch None {
    | Some(x) => assert(false)
    | None => ()
  };

(I am not sure all cases are unambiguous, I can investigate patching the parser).

@chenglou
Copy link
Member

chenglou commented Dec 14, 2017

Awesome! Though can we do like ocaml and have let%lwt, etc?
Or maybe it's easier to have this one land first?

@let-def
Copy link
Contributor Author

let-def commented Dec 14, 2017

@chenglou: I agree it would be nice, I will investigate!

@jordwalke
Copy link
Member

The convenient thing about OCaml's let%ppx form is that it conveys that the whole let is a ppx extension - completely replacing an AST node, as opposed to a "decorator" attached to another otherwise normal AST node.

For that Reason, I'm thinking that the prefix syntax you have here would make perfect sense for @ attributes:

@foo
let anotherThing = bar;

Even if ppx extensions use the more "embedded" syntax:

let%foo = bar;

For single expressions, the same lean bracketless ppx attributes would also be awesome:

let something = @attr myFunc();

@let-def
Copy link
Contributor Author

let-def commented Dec 14, 2017

@chenglou: I agree it would be nice, I will investigate!

@let-def let-def closed this Dec 14, 2017
@let-def let-def reopened this Dec 14, 2017
@let-def
Copy link
Contributor Author

let-def commented Dec 18, 2017

Ok, the last update add supports for fun%extend, function%extend, let%extend, switch%extend, try%extend, while%extend, for%extend, if%extend (I hope I didn't forget any :P?).

Plus non negligible simplification to the parser (more to come).

@hcarty
Copy link
Contributor

hcarty commented Dec 18, 2017

@let-def Looks like it covers if%extend too, correct?

This is really cool! ocamlformat just got extension point sugar support too so pleasantly formatted extension points for everyone!

@let-def
Copy link
Contributor Author

let-def commented Dec 19, 2017

@hcarty indeed, thanks I forgot to list that one :).

@hcarty
Copy link
Contributor

hcarty commented Dec 21, 2017

This looks amazing - I can't wait to try it out!

Copy link
Member

@jordwalke jordwalke left a comment

Choose a reason for hiding this comment

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

Thank You!
I've wanted this for a long time and so have a lot of other people.

@jordwalke jordwalke merged commit 28a8922 into reasonml:master Dec 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants