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

interleave whitespace between recursive let and module bindings #1990

Merged
merged 4 commits into from
Nov 14, 2018

Conversation

IwanKaramazow
Copy link
Contributor

Fixes #1958

@chenglou
Copy link
Member

chenglou commented Jun 8, 2018

Should we even allow whitespace between recursive let bindings? As much as possible I'd still like us to be opinionated on whitespace

@IwanKaramazow
Copy link
Contributor Author

I think we should for consistency, it would be a little bit weird if we didn't allow it here.
Imagine rewriting the Reason printer with recursive functions instead of using the object system:

let rec printExpression = (e) => beatifulPrinting(e)

and printPattern = (pat) => ...

and printModule = (m) => ...

Some space between the different bindings gives a little more room to breath

@chenglou
Copy link
Member

chenglou commented Jun 8, 2018

Would this actually happen though? Because the reality would be:

let rec printExpression = (e) => {
  beatifulPrinting(e)
  ...
}
and printPattern = (pat) => {
  ...
}
and printModule = (m) => {
   ...
}

Right?

@IwanKaramazow
Copy link
Contributor Author

IwanKaramazow commented Jun 9, 2018

Yes, but someone might want to write the following:

let rec main = (e) => {
  ...
}

and helper1 = (_) => {
  ...
}
and helper2 = (_) => {
   ...
}
and helper3 = (_) => {
   ...
}

helper1, helper2, helper3 could be grouped together. Visually it becomes very clear what the relation between the helper{X} and main is.

I might be completely wrong though, no opinion, and can close the PR.

@jaredly
Copy link
Contributor

jaredly commented Jun 13, 2018

I have a mild preference for keeping recursive bindings together

IwanKaramazow and others added 3 commits November 14, 2018 07:11
* Consistent with Prettier
* Wastes less whitespace

Before:
```reason
<div>
  {
    switch (color) {
    | Black => ReasonReact.string("black")
    | Red => ReasonReact.string("red")
    }
  }
</div>;
```

```reason
<div>
  {switch (color) {
   | Black => ReasonReact.string("black")
   | Red => ReasonReact.string("red")
   }}
</div>;
```
@IwanKaramazow IwanKaramazow force-pushed the WhitespaceRecursiveBindings branch from d1a4de8 to e542357 Compare November 14, 2018 06:49
@chenglou chenglou merged commit 3591742 into reasonml:master Nov 14, 2018
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.

4 participants