Allow single-line `if` expressions #648

rlefevre · 2019-11-16T20:49:31Z

Resolves #209

rlefevre · 2019-11-16T21:13:52Z

Note: I have not found a more optimized solution than the 3 trackNewline but I may have overlooked it.

tests/test-files/transform/Elm-0.18/Examples.formatted.elm

avh4

Thanks for taking this on!

This will need more tests. Specifically an example of all the different single/multiline combinations should be added to tests/test-files/good/Elm-0.19/AllSyntax/Expressions.elm (probably in or immediately after ifStatement (line 273)).

parser/src/Parse/Expression.hs

tests/test-files/transform/Elm-0.18/Examples.formatted.elm

rlefevre · 2019-11-17T18:49:03Z

I totally agree this needs more tests, but I need some guidance because I don't understand if tests in ExpressionTest.hs should use example that seem to only test parsing, example' that compares to the formatted string, or tests like tests/test-files/transform/Elm-0.18/Examples.formatted.elm that compare a full file. I'm a little confused.

avh4 · 2019-11-17T18:54:38Z

Ah, ExpressionTest.hs is meant to be phased out (as you've probably noticed, it's a pain to update). The tests/test-files/good is the main test suite, and every new feature should have tests there (for this change, I think Elm-0.19/AllSyntax/Expressions.elm makes sense. Then tests/test-files/transform is for things that require the input file to be different from the output file.

So for this PR:

add examples of the new allowed formats in tests/test-files/good/Elm-0.19/AllSyntax/Expressions.elm
update ExpressionTest.hs as little as possible so that it compiles and passes
If there's a tests/test-files/transform you want to add, you can, but it's not required

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

tests/test-files/transform/Elm-0.18/Examples.formatted.elm

src/ElmFormat/Render/Box.hs

tests added

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

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:22:40Z

tests/test-files/transform/Elm-0.19/IfElseIfAlwaysMultiLine.elm

@@ -0,0 +1 @@
+x = if True then 1 else if False then 2 else 3


avh4 · 2019-11-24T18:25:17Z

src/ElmFormat/Render/Box.hs

+                ]
+
+        (_, ifCond, thenBody, _, elseBody) ->
+            formatIfExpressionMultiline elmVersion importInfo ifCond thenBody elseifs elseBody


The way these two functions work together seems a bit complicated. The ideal is that these functions basically have two phases: 1) turning all the stuff into boxes and 2) assembling the boxes based on the multiline features. Here we turn just enough into boxes to decide if we should call the other function, which makes it a bit harder to follow the full logic.

Though looking at the code for a few minutes, I don't have any quick suggestions of how to refactor this, so if you don't have time to give a try to simplifying this more, that's fine. (A lot of the existing code in here is way more confusing than this)

I think it should be easier to follow the logic now.

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

Looks great, thanks! I'll let you know when I get upgrade-tool merged (goal is the next two weeks).

And also I think this will start the 0.9 releases as a notable behavior change.

rlefevre · 2019-11-25T14:17:11Z

src/ElmFormat/Render/Box.hs

+        Left box -> box
+
+
+boxesOrLinesToBox :: Bool -> Either [Box] [Line] -> Box


Maybe we could put this function and boxOrLineToBox into Box.hs?
I'm not sure because of the use of ElmStructure.forceableSpaceSepOrStack1.

Resolves avh4#209

andys8 · 2019-11-29T13:54:51Z

@rlefevre (and @avh4) Thanks so much for tackling this issue. It is really "improving the language" for me :)

ChristophP · 2020-09-24T18:44:59Z

Really looking forward to this feature. I feel like this would clean up a bunch of code the previously required lots of space

rlefevre mentioned this pull request Nov 16, 2019

Single/Multi-line formatting trigger discussion #647

Open

rlefevre force-pushed the single-line-if-expressions branch from e4fc755 to 51a20b7 Compare November 16, 2019 21:00

rlefevre mentioned this pull request Nov 16, 2019

Should single-line if expressions be allowed? #209

Closed

rlefevre force-pushed the single-line-if-expressions branch from 51a20b7 to a4c2d98 Compare November 16, 2019 21:10

rlefevre commented Nov 16, 2019

View reviewed changes

tests/test-files/transform/Elm-0.18/Examples.formatted.elm Outdated Show resolved Hide resolved

rlefevre force-pushed the single-line-if-expressions branch 3 times, most recently from 14298af to bb3f1c2 Compare November 16, 2019 23:50

rlefevre changed the title ~~Add support for single-line if expressions~~ Allow single-line if expressions Nov 17, 2019

rlefevre mentioned this pull request Nov 17, 2019

Remaining differences with elm/core #650

Open

avh4 previously requested changes Nov 17, 2019

View reviewed changes

avh4 reviewed Nov 17, 2019

View reviewed changes

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

avh4 reviewed Nov 17, 2019

View reviewed changes

tests/test-files/transform/Elm-0.18/Examples.formatted.elm Show resolved Hide resolved

avh4 added this to the 0.9.0-exp last experimental features before 1.0 milestone Nov 17, 2019

This comment has been minimized.

Sign in to view

rlefevre force-pushed the single-line-if-expressions branch from bb3f1c2 to 67161e6 Compare November 17, 2019 20:08

rlefevre commented Nov 17, 2019

View reviewed changes

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

rlefevre requested a review from avh4 November 17, 2019 20:20

rlefevre force-pushed the single-line-if-expressions branch 2 times, most recently from 31f33b7 to e83afa4 Compare November 17, 2019 22:44

avh4 reviewed Nov 18, 2019

View reviewed changes

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

rlefevre force-pushed the single-line-if-expressions branch from e83afa4 to c541461 Compare November 18, 2019 10:50

rlefevre commented Nov 18, 2019

View reviewed changes

tests/test-files/transform/Elm-0.18/Examples.formatted.elm Outdated Show resolved Hide resolved

rlefevre requested a review from avh4 November 18, 2019 11:19

rlefevre commented Nov 18, 2019

View reviewed changes

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

rlefevre force-pushed the single-line-if-expressions branch from c541461 to 12f4095 Compare November 18, 2019 17:37

rlefevre force-pushed the single-line-if-expressions branch 2 times, most recently from e4f3518 to 7ee8a14 Compare November 22, 2019 23:03

avh4 reviewed Nov 24, 2019

View reviewed changes

tests/test-files/transform/Elm-0.19/IfElseIfAlwaysMultiLine.elm

@@ -0,0 +1 @@

x = if True then 1 else if False then 2 else 3

Copy link

Owner

avh4 Nov 24, 2019

Choose a reason for hiding this comment

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

👍

avh4 reviewed Nov 24, 2019

View reviewed changes

rlefevre force-pushed the single-line-if-expressions branch 2 times, most recently from dafb82b to d0ec5dc Compare November 25, 2019 12:05

rlefevre commented Nov 25, 2019

View reviewed changes

rlefevre added 2 commits November 25, 2019 17:21

Allow single-line if expressions

657c586

Resolves avh4#209

Add consecutive comments tests for if expressions

d9404ad

rlefevre force-pushed the single-line-if-expressions branch from d0ec5dc to d9404ad Compare November 25, 2019 16:22

avh4 self-assigned this Apr 27, 2020

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 `if` expressions #648

Allow single-line `if` expressions #648

rlefevre commented Nov 16, 2019 •

edited

rlefevre commented Nov 16, 2019 •

edited

avh4 left a comment

rlefevre commented Nov 17, 2019

avh4 commented Nov 17, 2019

This comment has been minimized.

rlefevre commented Nov 22, 2019

avh4 Nov 24, 2019

avh4 Nov 24, 2019

rlefevre Nov 25, 2019

avh4 commented Nov 24, 2019

rlefevre Nov 25, 2019 •

edited

andys8 commented Nov 29, 2019

ChristophP commented Sep 24, 2020

		Left box -> box


		boxesOrLinesToBox :: Bool -> Either [Box] [Line] -> Box

Allow single-line if expressions #648

Are you sure you want to change the base?

Allow single-line if expressions #648

Conversation

rlefevre commented Nov 16, 2019 • edited

rlefevre commented Nov 16, 2019 • edited

avh4 left a comment

Choose a reason for hiding this comment

rlefevre commented Nov 17, 2019

avh4 commented Nov 17, 2019

This comment has been minimized.

rlefevre commented Nov 22, 2019

avh4 Nov 24, 2019

Choose a reason for hiding this comment

avh4 Nov 24, 2019

Choose a reason for hiding this comment

rlefevre Nov 25, 2019

Choose a reason for hiding this comment

avh4 commented Nov 24, 2019

rlefevre Nov 25, 2019 • edited

Choose a reason for hiding this comment

andys8 commented Nov 29, 2019

ChristophP commented Sep 24, 2020

Allow single-line `if` expressions #648

Allow single-line `if` expressions #648

rlefevre commented Nov 16, 2019 •

edited

rlefevre commented Nov 16, 2019 •

edited

rlefevre Nov 25, 2019 •

edited