reason Preserve function body braces and distinguish open vs. X.()

Summary:Created a new place to store some stylistic information in the AST. [@refmt "foo"] attributes used at parse time, never printed.

Before the following would get reformatted:

let f () => {
  switch(x) {
  | X => 0
  };
};

To this:

let f () =>
  switch(x) {
  | X => 0
  };

That was annoying to many people because you can't easily add a "let" binding above the switch. After this diff, the original form will be preserved:

let f () => {
  switch(x) {
  | X => 0
  };
};

With this diff you can still have the printer print this form if you prefer: You simply write it in this form and it will be retained.

let f () =>
  switch(x) {
  | X => 0
  };

I consider this a "pretty good" solution to making editing Reason syntax more enjoyable, but is certainly not the best possible solution. Ideally we would automatically insert the braces depending on whether or not the body of the function "breaks". We can also (or even instead) do that when our formatter is more expressive.

Before the following would get reformatted:

let f () => {
  open X;
  switch(x) {
  | X => 0
  };
};

To this:

let f () =>
  X.(
    switch(x) {
    | X => 0
    }
  );

Which is sometimes neat, but sometimes it gets pretty unwieldy especially with nested opens. With this diff, we preserve the original way you wrote the open. You can write it with open X or X.() and the formatter will retain the form you wrote. It's pretty hard to write an algorithm that looks great in all cases.

Feb 16 '18 09:02 jordwalke

Some people that have requested this: @sgrove, @kyldvs

Feb 16 '18 09:02 jordwalke

While I really like the local open change, I’m less sold on retaining optional braces. I usually like the way simplifying a function can lead to the braces becoming unnecessary and collapse the code; often into a single line.

Although I guess it’s easy to argue both sides. This does make me think that reducing how opinionated refmt is means we start needing some options to control behaviour 🤔

Feb 17 '18 03:02 TheSpyder

The open brace issue is pretty important because they get removed before you have the opportunity to add a let binding before the return value. Today people already do force the braces to be preserved by adding a dummy let binding to unit. That’s pretty extreme. What this diff does is just makes it more natural to tell the formatter you want them preserved. However I think that if we had the ability to only print braces when the function body breaks, then this feature would be less useful and at that time perhaps we can revisit.

Feb 17 '18 05:02 jordwalke

I run into the bracket issue all the time, particularly because I'm used to a format-on-save workflow.

let foo = (foo) => {
  /* some placeholder */
};

Will format out the braces and I have to add them back constantly. Typically my workaround is to always add some empty unit statements to the function body like this:

let foo = (foo) => {
  ();
  /* some placeholder */
};

Really looking forward to this change

Feb 20 '18 20:02 kyldvs

@kyldvs Literally my exact workflow! Looking forward to it as well.

Feb 20 '18 20:02 sgrove

@jordwalke are you still planning to land this (or something similar)? I've been encountering both behaviors you outlined originally and it makes editing in vscode with formatOnSave: true very jarring. Often I'll habitually save, get one of those reformats when I planned on editing in the block and have to undo the format (which sometimes ends in a weird result).

Jul 08 '18 19:07 phated

This is the main reason I have refmt on save turned off. I might try to get to this to resolve the conflicts if I can in the next couple weeks

Jul 19 '18 21:07 jaredly

@jaredly Just an observation: I felt that the refactor included in this diff was really necessary to make it easy to retain these "preserved styles". I found the current approach in the printer/parser not really sustainable and kind of fragile. With my refactor in this stack, there's a specific place carved out just for stylistic preservation and it is clearly out of the way because it's its own category that is cleanly filtered out.

Jul 20 '18 08:07 jordwalke

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.