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

Revisit if/then/else #2334

Merged
merged 5 commits into from
Jul 7, 2022
Merged

Revisit if/then/else #2334

merged 5 commits into from
Jul 7, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jul 4, 2022

This PR introduces the fsharp_max_if_then_short_width setting from #2299 and revisits long ifExpr and matchExpr (See fsharp/fslang-design#646 (comment)).

My personal interest is more in the latter, as it is quite satisfying how much more consistently we deal with these situations.

The flow is the following:

if x then y

First if x then is printed, if it fits on the rest of the line it stays in one line.
If it cross the max_line_length boundary or is multiline it changes to

if
   x
then
   y

If the entire expression is lower than the fsharp_max_if_then_short_width, it stays in one line.
Otherwise, it will put the thenExpr on the next line:

if x then
   y

The default value for this is 0, so out of the box, it will always format this on multiple lines.


The same flow applies when there is an elseExpr, how there the existing fsharp_max_if_then_else_short_width setting is used there.

if x then y else z

// or

if x then
   y
else
   z

// or

if
   x
then
   y
else
   z

In case there are any elif branches, we check if every line is short. If so, we apply the short style otherwise the long one. Again, what happens inside the thenExpr will be respected.

if a then b
elif c then d
else if e then f

// Not that we use `fsharp_max_if_then_short_width` to check if they all fit on one line

// if not ...

if a then
   b
elif
    c // in case c is too long or multiline
then
   d
else if e then
   f

The same flow applies if there is an elseExpr, however, in there we use the fsharp_max_if_then_else_short_width length.


The same principle applies to match and match! expressions:

match x with
| _ -> ()

// if x is too long...

match 
    x
with
| _ -> ()

The nice thing is that x can be formatted the exact same way it would be as a standalone expression. This is because of the relaxations in F# 6 and sparks a lot of joy for this maintainer.

The story overall makes a lot more sense when we do the multiline thing for ifExpr or matchExpr.

@dsyme what do you think about this?
@dawedawe would you mind reviewing this?

@nojaf nojaf force-pushed the poc/if-then-else branch from c4bdbdf to 99fc626 Compare July 4, 2022 07:47
Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

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

Nice!

src/Fantomas.Core/CodePrinter.fs Show resolved Hide resolved
src/Fantomas.Core/CodePrinter.fs Show resolved Hide resolved
@dsyme
Copy link
Contributor

dsyme commented Jul 4, 2022

Looks great (from PR description)

Should fsharp_max_match_short_width be added to allow single line match x with A -> true | _ -> false and make it all symmetric?

@nojaf
Copy link
Contributor Author

nojaf commented Jul 5, 2022

Should fsharp_max_match_short_width be added to allow single line match x with A -> true | _ -> false and make it all symmetric?

This never came up in Fantomas or the style guide so I don't think the community is really waiting for that setting. If enough interest is expressed in the style guide we could have this, but for now, it feels like we can do without it.

I'll start raising a PR for the style guide to capture what is implemented here. Once that lands I'll finish up this PR.

@nojaf nojaf force-pushed the poc/if-then-else branch from 088534a to dbd640f Compare July 6, 2022 15:51
@nojaf nojaf force-pushed the poc/if-then-else branch from dbd640f to 8d92dd2 Compare July 7, 2022 06:46
@nojaf nojaf changed the title POC: revisit if/then/else Revisit if/then/else Jul 7, 2022
@nojaf nojaf merged commit 8cc88bf into fsprojects:master Jul 7, 2022
@nojaf nojaf deleted the poc/if-then-else branch July 7, 2022 07:45
@@ -442,6 +443,26 @@ let a = [1;2;3]
let b = [|4;5;6|]
```

### fsharp_max_if_then_short_width
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yisusalanpng keep in mind that I added this documentation bit to the old docs.
Please move it as well in #2336

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