Keep tuple trivia while formatting #1365
Conversation
Evangelink
changed the title
There was a problem hiding this comment.
I have left 2 open questions with potential bugs, if you confirm they are bugs let me know if you want them fix part of this PR or through another one.
Also I wanted to say a big thank you, that was a really good experience working on this project. Guidelines are clear, you have provided most of the info so that even a beginner can feel confident to jump-in. That's really appreciated and a sign of a really good "management". Keep-up this awesome spirit.
src/Fantomas.Tests/TupleTests.fs
Outdated
|
|
||
| func ( | ||
| // abc | ||
| 0, // def |
There was a problem hiding this comment.
This looks like a bug, I would have expected to have either only one space or to have also two spaces for // jkl below.
There was a problem hiding this comment.
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 rprNotice 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.
| 0, // def | ||
| // ghi | ||
| 1 // jkl | ||
| // mno |
There was a problem hiding this comment.
Let me know if this isn't the expected location for this comment trivia.
There was a problem hiding this comment.
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.
src/Fantomas/CodePrinter.fs
Outdated
| @@ -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) -> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Hey @Evangelink, this looks good! |
Fix #1350