Allow single-line case-expressions branches #649

rlefevre · 2019-11-16T23:53:50Z

Résolves #507

rlefevre · 2019-11-16T23:54:54Z

Note:

~~This implementation uses a variant of the method proposed in Single/Multi-line formatting trigger discussion #647 to trigger the single-line branches.~~
This implementation does not align arrows

rlefevre · 2019-11-17T21:46:01Z

Updated with Multiline instead of Bool, and tests added.

rlefevre · 2019-11-18T11:12:59Z

@avh4 Given what's accepted with single-line if expressions, do we want to accept single-line comments in branches like the following example?

case Just 1 of
    Just x -> {- A -} x
    _ -> {- B -} 2

For now this is not supported in this PR (comments lead to multi-line except those between case...of).

avh4 · 2019-11-18T16:26:57Z

do we want to accept single-line comments

yes, I think so

src/ElmFormat/Render/Box.hs

tests/test-files/good/Elm-0.19/AllSyntax/Expressions.elm

rlefevre · 2019-11-22T23:20:07Z

I think that the PR is now complete, including tests.
I will just rebase it once the upgrade-tool branch is merged.

avh4 · 2019-11-24T18:30:53Z

so several comments are on the same line, whereas for case branches, we get

I suspect there was not a reason for that difference.

case Just 1 of
    {- A -} Just x {- B -} -> {- C -} x

comments in position A are very rare from code I've observed, so I don't think it really matters how we choose to deal with these.

parser/src/Parse/Expression.hs

avh4 · 2019-11-24T18:36:40Z

parser/src/Parse/Expression.hs

-      result <- cases firstPatternComments
-      return $ E.Case (e, multilineToBool multilineSubject) result
+      (branches, multilineFirstBranch) <- cases firstPatternComments
+      return $ E.Case (e, multilineSubject) (branches, multilineFirstBranch)


Was it in a separate issue you were suggesting using the linebreak of the first item to control multiline for other kinds of expressions besides cases? I'm still hesitant about that. I'd be more comfortable having this only be JoinAll if all of the branches are single line, and then maybe doing a separate experiment after adding this to try the idea of having the first item control everything.

I thought we could experiment it with only case expressions, but you're right, it's better to introduce the single if and case expressions in a consistent way with existing behavior, then discuss eventually the other method. I will make the change.

New version uses JoinAll if all branches are single line.

avh4 · 2019-11-24T18:39:11Z

src/ElmFormat/Render/Box.hs

+
+        (multilineBranches, branches') =
+            List.mapAccumR
+            (\mline (AST.Commented prePat pat postPat, (preRes, res)) ->


not necessary to change before merging, but FYI my preferred style here is to take this anonymous function and define it in the outermost let block right above the (multilineBranches, branches') = definition.

avh4 · 2019-11-24T18:41:55Z

src/ElmFormat/Render/Box.hs

+                    preRes' =
+                        Maybe.maybeToList $ formatComments preRes
+                    res' =
+                        formatExpression elmVersion importInfo SyntaxSeparated res


res/res'/preRes should I think be called "body" instead of "res" to be consistent with other code, unless there was a specific reason you chose that name.

avh4 · 2019-11-24T18:42:50Z

src/ElmFormat/Render/Box.hs

+    opening
+        |> andThen
+            (branches'
+                |> map (branch multilineBranches)


Not necessary to change before merging, but FYI I'm trying to prefer fmap over map (just so that people new to the haskell don't have to wonder what the difference is)

avh4 · 2019-11-24T18:45:03Z

src/ElmFormat/Render/Box.hs

+            (branches'
+                |> map (branch multilineBranches)
+                |> (if AST.isMultiline multilineBranches then List.intersperse blankLine else id)
+                |> map indent


Will this indent the interspersed blank lines? I can't remember if that matters... if this ends up leaving empty lines containing spaces, then that needs to be avoided

Good point. I will check.

I tested and blank lines are trimmed to only "\n".
I have not checked where though, most likely during Box -> Text rendering.

avh4 · 2019-11-24T18:49:55Z

src/ElmFormat/Render/Box.hs

+
+        branch multiline' (singles, prePat, pat, postPat, preRes, res) =
+            case (multiline', singles, prePat, pat, postPat) of
+                (AST.JoinAll, Right singleLines, _, _, _) ->


I see my previous code was similarly confusing, but it's not obvious here that singleLines includes the data from pat, postPat, preRes, res. Could you at least add a comment right above this case noting that? (without it, I started reading these branches and though "oh no, is some of the AST getting lost here?")

I have reworked this part and it is hopefully a lot less confusing.
I used an Either to differentiate between single lines and boxes to avoid the variables overlap.

Resolves avh4#507

tommyengstrom · 2019-12-05T12:51:02Z

Nice! I was just about to stop using elm-format after having used it for some time. I find that all the extra lines makes the code much harder to read than if I wasn't using anything at all. This fixes the main problem. 👍

rlefevre force-pushed the single-line-case-expressions branch 2 times, most recently from 74bfb5a to 1d1ef0b Compare November 17, 2019 07:42

This was referenced Nov 17, 2019

Remaining differences with elm/core #650

Open

Single/Multi-line formatting trigger discussion #647

Open

rlefevre force-pushed the single-line-case-expressions branch from 1d1ef0b to 163199f Compare November 17, 2019 21:23

This comment has been minimized.

Sign in to view

rlefevre force-pushed the single-line-case-expressions branch 2 times, most recently from 7b7560b to 95f70f9 Compare November 22, 2019 23:12

rlefevre commented Nov 22, 2019

View reviewed changes

src/ElmFormat/Render/Box.hs Show resolved Hide resolved

tests/test-files/good/Elm-0.19/AllSyntax/Expressions.elm Show resolved Hide resolved

avh4 reviewed Nov 24, 2019

View reviewed changes

parser/src/Parse/Expression.hs Show resolved Hide resolved

avh4 reviewed Nov 24, 2019

View reviewed changes

avh4 added this to the 0.9.0-exp milestone Nov 24, 2019

rlefevre force-pushed the single-line-case-expressions branch 2 times, most recently from 52e8480 to 93c89e4 Compare November 26, 2019 00:19

rlefevre added 2 commits November 26, 2019 01:25

Allow single-line case-expressions branches

ed45631

Resolves avh4#507

Add consecutive comments tests for case expressions

efbecb9

rlefevre force-pushed the single-line-case-expressions branch from 93c89e4 to efbecb9 Compare November 26, 2019 00:25

avh4 changed the base branch from master to main September 18, 2020 02:54

avh4 added the 0.9 (temporary label for search filtering) label Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow single-line case-expressions branches #649

Allow single-line case-expressions branches #649

rlefevre commented Nov 16, 2019 •

edited

rlefevre commented Nov 16, 2019 •

edited

rlefevre commented Nov 17, 2019

rlefevre commented Nov 18, 2019

avh4 commented Nov 18, 2019

This comment has been minimized.

rlefevre commented Nov 22, 2019

avh4 commented Nov 24, 2019 •

edited

avh4 Nov 24, 2019

rlefevre Nov 24, 2019

rlefevre Nov 26, 2019

avh4 Nov 24, 2019

rlefevre Nov 26, 2019

avh4 Nov 24, 2019

rlefevre Nov 26, 2019

avh4 Nov 24, 2019 •

edited

rlefevre Nov 26, 2019

avh4 Nov 24, 2019 •

edited

rlefevre Nov 24, 2019

rlefevre Nov 26, 2019

avh4 Nov 24, 2019

rlefevre Nov 26, 2019

tommyengstrom commented Dec 5, 2019

Allow single-line case-expressions branches #649

Are you sure you want to change the base?

Allow single-line case-expressions branches #649

Conversation

rlefevre commented Nov 16, 2019 • edited

rlefevre commented Nov 16, 2019 • edited

rlefevre commented Nov 17, 2019

rlefevre commented Nov 18, 2019

avh4 commented Nov 18, 2019

This comment has been minimized.

rlefevre commented Nov 22, 2019

avh4 commented Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avh4 Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avh4 Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommyengstrom commented Dec 5, 2019

rlefevre commented Nov 16, 2019 •

edited

rlefevre commented Nov 16, 2019 •

edited

avh4 commented Nov 24, 2019 •

edited

avh4 Nov 24, 2019 •

edited

avh4 Nov 24, 2019 •

edited