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.

Sign up for GitHub

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

Keep tuple trivia while formatting #1365

Merged
merged 8 commits into from
Jan 15, 2021
Merged

Keep tuple trivia while formatting #1365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5930e3f
Keep tuple trivia while formatting
Evangelink Jan 13, 2021
b767223
Fix issue and add one extra test
Evangelink Jan 15, 2021
df92bd3
Merge branch 'master' into comment-tuple
Evangelink Jan 15, 2021
1fc4e4a
Fix formatting
Evangelink Jan 15, 2021
e818b29
Merge branch 'comment-tuple' of https://github.com/Evangelink/fantoma…
Evangelink Jan 15, 2021
ba82f4a
Merge branch 'master' into comment-tuple
Evangelink Jan 15, 2021
284c9b1
Rename rangeOfTuple into tupleRange
Evangelink Jan 15, 2021
a26dca9
Fix spacing issue
Evangelink Jan 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/Fantomas.Tests/TupleTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,3 +268,61 @@ let y =
sprintf "(%s)" args),
namesWithIndices
"""

[<Test>]
let ``comment on first tuple argument is preserved`` () =
formatSourceString
false
"""
let func (a, b) = a + b

func(
// abc
0,
1
)
"""
config
|> prepend newline
|> should
equal
"""
let func (a, b) = a + b

func (
// abc
0,
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
1
)
"""

[<Test>]
let ``comment trivias on tuple arguments are preserved`` () =
formatSourceString
false
"""
let func (a, b) = a + b

func(
// abc
0, // def
// ghi
1 // jkl
// mno
)
"""
config
|> prepend newline
|> should
equal
"""
let func (a, b) = a + b

func (
// abc
0, // def
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a bug, I would have expected to have either only one space or to have also two spaces for // jkl below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot, this can be solved by changing long to

            let long =
                genExpr astContext e
                +> sepSpace
                +> sepOpenTFor lpr
                +> indent
                +> sepNln
                +> (col (sepCommaFixed +> sepNln) args (genExprLong astContext)
                    |> genTupleTrivia)
                +> unindent
                +> sepNln
                +> sepCloseTFor rpr

Notice that sepCommaFixed does not exist yet.
You can add it in Context (right under sepComma):

let internal sepCommaFixed = str ","

The Fixed convention means, give me the value without checking the configuration if there should be a space.

// ghi
1 // jkl
// mno
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this isn't the expected location for this comment trivia.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is another bug related to the assignment of trivia to a trivia node.
In short, Fantomas detected the comment // mno and has to find an anchor to attach it.
It chose to add it before the closing parenthesis ) instead of after the SynExpr.Tuple.
I'm aware of these situations, but it is quite complex to solve to be fair.
For this PR you can leave it like that.

)
"""
13 changes: 9 additions & 4 deletions src/Fantomas/CodePrinter.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,7 @@ and genExpr astContext synExpr ctx =
+> sepCloseTFor rpr

expressionFitsOnRestOfLine short long
| Tuple es -> genTuple astContext es
| Tuple (es, _) -> genTuple astContext es
| StructTuple es ->
!- "struct "
+> sepOpenT
Expand Down Expand Up @@ -1899,7 +1899,7 @@ and genExpr astContext synExpr ctx =

isShortExpression ctx.Config.MaxDotGetExpressionWidth (genExpr sepNone) (genExpr sepNln) ctx

| AppTuple (e, lpr, args, rpr) when (not astContext.IsInsideDotGet) ->
| AppTuple (e, lpr, args, rangeOfTuple, rpr) when (not astContext.IsInsideDotGet) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used rangeOfTuple as suggested but I have seen you are using both rangeOfXXX and XXXRange notation in the code so let me know if you prefer the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, good remark. Perhaps go for tupleRange instead.
Both do appear in the code base indeed, this hasn't really been a conscious decision I think.

let sepSpace (ctx: Context) =
if astContext.IsInsideDotGet
|| astContext.IsInsideDotIndexed then
Expand All @@ -1922,11 +1922,16 @@ and genExpr astContext synExpr ctx =

expr e

let genTupleTrivia f =
match rangeOfTuple with
| Some range -> f |> genTriviaFor SynExpr_Tuple range
| None -> f

let short =
genExpr astContext e
+> sepSpace
+> sepOpenTFor lpr
+> col sepComma args (genShortExpr astContext)
+> (col sepComma args (genShortExpr astContext) |> genTupleTrivia)
+> sepCloseTFor rpr

let long =
Expand All @@ -1935,7 +1940,7 @@ and genExpr astContext synExpr ctx =
+> sepOpenTFor lpr
+> indent
+> sepNln
+> col (sepComma +> sepNln) args (genExprLong astContext)
+> (col (sepComma +> sepNln) args (genExprLong astContext) |> genTupleTrivia)
+> unindent
+> sepNln
+> sepCloseTFor rpr
Expand Down
10 changes: 5 additions & 5 deletions src/Fantomas/SourceParser.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ let (|ArrayOrList|_|) =

let (|Tuple|_|) =
function
| SynExpr.Tuple (false, exprs, _, _) -> Some exprs
| SynExpr.Tuple (false, exprs, _, rangeOfTuple) -> Some(exprs, Some rangeOfTuple)
| _ -> None

let (|StructTuple|_|) =
Expand Down Expand Up @@ -800,17 +800,17 @@ let (|App|_|) e =
let (|AppTuple|_|) =
function
| App (SynExpr.DotGet _, [ (Paren (_, Tuple _, _)) ]) -> None
| App (e, [ (Paren (lpr, Tuple args, rpr)) ]) -> Some(e, lpr, args, rpr)
| App (e, [ (Paren (lpr, Tuple (args, rangeOfTuple), rpr)) ]) -> Some(e, lpr, args, rangeOfTuple, rpr)
| App (e, [ (Paren (lpr, singleExpr, rpr)) ]) ->
match singleExpr with
| SynExpr.Lambda _
| SynExpr.MatchLambda _ -> None
| _ -> Some(e, lpr, [ singleExpr ], rpr)
| _ -> Some(e, lpr, [ singleExpr ], None, rpr)
| _ -> None

let (|NewTuple|_|) =
function
| SynExpr.New (_, t, Paren (lpr, Tuple args, rpr), _) -> Some(t, lpr, args, rpr)
| SynExpr.New (_, t, Paren (lpr, Tuple (args, _), rpr), _) -> Some(t, lpr, args, rpr)
| SynExpr.New (_, t, Paren (lpr, singleExpr, rpr), _) -> Some(t, lpr, [ singleExpr ], rpr)
| SynExpr.New (_, t, ConstExpr (SynConst.Unit, unitRange), _) ->
let lpr =
Expand Down Expand Up @@ -841,7 +841,7 @@ let (|PrefixApp|_|) =

let (|InfixApp|_|) synExpr =
match synExpr with
| SynExpr.App (_, true, (Var "::" as e), Tuple [ e1; e2 ], _) -> Some("::", e, e1, e2)
| SynExpr.App (_, true, (Var "::" as e), Tuple ([ e1; e2 ], _), _) -> Some("::", e, e1, e2)
// Range operators need special treatments, so we exclude them here
| SynExpr.App (_, _, SynExpr.App (_, true, (Var s as e), e1, _), e2, _) when s <> ".." -> Some(s, e, e1, e2)
| _ -> None
Expand Down