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

Fix: print line comment after arrow in lamda between paranthesis #557

Merged
merged 10 commits into from
Nov 22, 2019

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Nov 9, 2019

In #539 I tried to fix bug #534 and by I ended up with two broken tests that were somewhat strange and debatable.

This PR contains the same fix for #534 and also refactors the way SynExpr.IfThenElse is formatted.
As close to the F# style guide as possible.

In the case of

If either cond, e1 or e2 are longer, but not multi-line

the guide leaves some room for interpretation. What is "longer" exactly?
Because of this question I introduced a new setting MaxIfThenElseShortWidth to let the user control this.

The main motivation to rewrite the formatting of SynExpr.IfThenElse is the support for comments around keywords (if, elif, else if), this have been tested extensively in IfThenElseTests.fs.

// Comment 1
if b then ()
// Comment 2
else ()
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO there should be indent after else.

then // c3
b // c4
else // c5
if // c6
Copy link
Contributor

Choose a reason for hiding this comment

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

no indent after else

if a then
b
else // meh
if c then
Copy link
Contributor

Choose a reason for hiding this comment

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

no indent after else

@nojaf
Copy link
Contributor Author

nojaf commented Nov 20, 2019

The no indentation after the else part was intended after reading the style guide

Multiple conditionals with elif and else are indented at the same scope as the if:

I consider else if and elif to be the same thing.

The fsharp lang ref seems to confirm this.

When chaining if...then...else expressions together, you can use the keyword elif instead of else if; they are equivalent.

So that is why no indentation.

In an extreme example I wouldn't imagine you would write the following code like this right?

if aaaa then
    meh
else 
    if bbbb then
        fooo
    else
        if ccc then
            baaarrr
        else
            if dddd then
                meehhh
            else
                ()

@jindraivanek
Copy link
Contributor

Multiple conditionals with elif and else are indented at the same scope as the if:

IMO this means only that elif and else is in same indentation as if not the block after else.

The fsharp lang ref seems to confirm this.

I know it is valid code, I just always considered this as bad style.

But "it is same as elif" is quite good argument.

In an extreme example I wouldn't imagine you would write the following code like this right?

Yeah, I would write it with elif ;)


To move forward, I think we can merge this, and raise an issue in style docs to clarify this.

@nojaf
Copy link
Contributor Author

nojaf commented Nov 21, 2019

I've created dotnet/docs#15922, we can always later change the formatting to whatever the outcome ends up being.
@jindraivanek could you approve to move forward?

@jindraivanek
Copy link
Contributor

@nojaf Thanks!

# Conflicts:
#	src/Fantomas.Tests/LambdaTests.fs
#	src/Fantomas/CodePrinter.fs
@nojaf nojaf merged commit 818c5a9 into fsprojects:master Nov 22, 2019
@nojaf nojaf deleted the fix-534-2 branch November 22, 2019 09:48
@knocte
Copy link
Contributor

knocte commented Dec 2, 2019

After this PR has been merged, is there a way to configure fantomas to make ifs always multi-line?

@nojaf
Copy link
Contributor Author

nojaf commented Dec 2, 2019

@knocte try setting --maxIfThenElseShortWidth to 0.

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.

None yet

3 participants