Allow single-line `if` expressions by rlefevre · Pull Request #648 · avh4/elm-format

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.

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)).

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 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

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


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

+                ]
+
+        (_, 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

+        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

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

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

Comment thread parser/src/Parse/Expression.hs Outdated

avh4 reviewed Nov 17, 2019

View reviewed changes

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

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

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

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

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

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

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

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

rlefevre commented Nov 18, 2019

View reviewed changes

Comment thread src/ElmFormat/Render/Box.hs Outdated

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

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

		Left box -> box


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

Conversation

rlefevre commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlefevre commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

avh4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rlefevre commented Nov 17, 2019

Uh oh!

avh4 commented Nov 17, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rlefevre commented Nov 22, 2019

Uh oh!

avh4 Nov 24, 2019

Choose a reason for hiding this comment

Uh oh!

avh4 Nov 24, 2019

Choose a reason for hiding this comment

Uh oh!

rlefevre Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

avh4 commented Nov 24, 2019

Uh oh!

rlefevre Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andys8 commented Nov 29, 2019

Uh oh!

ChristophP commented Sep 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rlefevre commented Nov 16, 2019 •

edited

Loading

rlefevre commented Nov 16, 2019 •

edited

Loading

rlefevre Nov 25, 2019 •

edited

Loading