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

Always add a space for multiple curried args invocation #2077

Closed
wants to merge 1 commit into from
Closed

Always add a space for multiple curried args invocation #2077

wants to merge 1 commit into from

Conversation

janus
Copy link
Contributor

@janus janus commented Feb 10, 2022

Addresses dotnet/docs#28141

@nojaf
Copy link
Contributor

nojaf commented Feb 11, 2022

Hello, thank you for this contribution.

Could you please first open an issue in Fantomas instead of adding a link to a PR to the docs.
This helps for the release notes later on.

Next, please target the 4.7 branch instead of the master.
As you could not have known this, I've added some additional notes in #2078.

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

       | AppWithLambda (e, es, lpr, lambda, rpr, pr) ->

should be updated as well to reflect these changes.
Something like:

            let sepSpaceAfterFunctionName =
                let sepSpaceBasedOnSetting e =
                    match e with
                    | Paren _ -> sepSpace
                    | UppercaseSynExpr -> (fun ctx -> onlyIf ctx.Config.SpaceBeforeUppercaseInvocation sepSpace ctx)
                    | LowercaseSynExpr -> (fun ctx -> onlyIf ctx.Config.SpaceBeforeLowercaseInvocation sepSpace ctx)

                match es with
                | [] -> sepSpaceBasedOnSetting e
                | _ -> sepSpace

@@ -213,3 +213,22 @@ let ``comment after address of tokens`` () =
&& // comment
foobar
"""

[<Test>]
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should not be in SingleExprTests.fs, a better candidate would be AppTests.fs.

@@ -3215,6 +3215,10 @@ and genApp astContext e es ctx =
(fun ctx ->
match es with
| [] -> false
| Paren _ :: rest when es.Length > 1 ->
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, as soon as there are more than elements in es it should return true.
Unit test maintain indent if when condition is multiline should be updated as well.

Copy link
Contributor Author

@janus janus Feb 11, 2022

Choose a reason for hiding this comment

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

@nojaf
It returns true if there are at least two curried args. If this fails, it continues with old logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for the old logic, that is my point.

@knocte
Copy link
Contributor

knocte commented Feb 13, 2022

Addressed comments in PR2088 (#2088) that supersedes this one.

@janus janus closed this Feb 13, 2022
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