Router: apply select indent after a comment #1334 #1557
Conversation
|
Thank you for this contribution! I really appreciate you taking a look at these long standing issues 🙏 At a quick glance this looks great but I won't be able to review properly until next week earliest. In the meantime, can you please update the PR descriptions to include before/after examples. I personally try to structure my PR descriptions in the following way
You can also include "Fixes #ISSUE" in the description so that the issue is automatically closed when merged. |
| @@ -62,7 +62,7 @@ val lastToken = owner.body.tokens.filter { | |||
| case _: Whitespace | _: Comment => false | |||
| case _ => true | |||
| } // edge case, if body is empty expire on arrow. | |||
| .lastOption.getOrElse(arrow) | |||
| .lastOption.getOrElse(arrow) | |||
There was a problem hiding this comment.
IMHO previous behavior was prettier 🤔
@tanishiking @olafurpg what do you think?
There was a problem hiding this comment.
I think it's acceptable to add an indent here 👍 , and it's more consistent if scalafmt adds an indent before methods for any objects (even if it's Block).
| @@ -1344,8 +1344,12 @@ class Router(formatOps: FormatOps) { | |||
| case FormatToken(_, c: T.Comment, between) => | |||
| Seq(Split(newlines2Modification(between), 0)) | |||
| // Commented out code should stay to the left | |||
| case FormatToken(c: T.Comment, _, between) if c.syntax.startsWith("//") => | |||
| Seq(Split(Newline, 0)) | |||
| case FormatToken(c: T.Comment, right, _) if c.syntax.startsWith("//") => | |||
There was a problem hiding this comment.
Could you add some comments that explain the behavior with some code examples? So that people who read this code can easily understand the meaning.
| @@ -37,3 +37,15 @@ class ExtendTest | |||
| with D | |||
| with E => | |||
| } | |||
| <<< #1334 1 | |||
There was a problem hiding this comment.
It might be better to move these tests to test/resources/default/indent.stat or something because this behavior is nothing to do with continuationIndent?
There was a problem hiding this comment.
no problem. perhaps i misunderstand the meaning of continuation; i thought that since we are indenting because of the .something which continues from previous line, it should go in this file.
| @@ -37,3 +37,15 @@ class ExtendTest | |||
| with D | |||
| with E => | |||
| } | |||
| <<< #1334 1 | |||
There was a problem hiding this comment.
it'll be helpful if you name the test with more meaningful description (you can include #1334 to point the issue number of course :) )
There was a problem hiding this comment.
that was an oversight, thanks for noticing.
There was a problem hiding this comment.
i will implement the suggestions shortly.
but i do want to get your opinion: while this fix appears to work, is it possible that the original problem is due to :chain: no looking far enough to include that line? there's a rule which obtains the method chain and inserts newlines/indents.
| @@ -1344,8 +1344,12 @@ class Router(formatOps: FormatOps) { | |||
| case FormatToken(_, c: T.Comment, between) => | |||
| Seq(Split(newlines2Modification(between), 0)) | |||
| // Commented out code should stay to the left | |||
| case FormatToken(c: T.Comment, _, between) if c.syntax.startsWith("//") => | |||
| Seq(Split(Newline, 0)) | |||
| case FormatToken(c: T.Comment, right, _) if c.syntax.startsWith("//") => | |||
| @@ -37,3 +37,15 @@ class ExtendTest | |||
| with D | |||
| with E => | |||
| } | |||
| <<< #1334 1 | |||
There was a problem hiding this comment.
no problem. perhaps i misunderstand the meaning of continuation; i thought that since we are indenting because of the .something which continues from previous line, it should go in this file.
| @@ -37,3 +37,15 @@ class ExtendTest | |||
| with D | |||
| with E => | |||
| } | |||
| <<< #1334 1 | |||
There was a problem hiding this comment.
that was an oversight, thanks for noticing.
Currently, the code doesn't introduce newlines into select chains even if their length exceeds the maximum line length. It also results in missing indent when a comment is in the middle of such a chain. Fix both problems, by allowing a newline if one is present in the input. Fixes scalameta#1334.
kitbellew
force-pushed
the
indent-chain-after-comment
branch
from
8532f6b to
bef40c2
Compare
| val vv = | ||
| v.aaaaaaaaaa.bbbbbbbbbb.cccccccccc.dddddddddd.eeeeeeeeee.ffffffffff.gggggggggg.hhhhhhhhhh.iiiiiiiiii | ||
| } | ||
| <<< #1334 very long broken select chain |
There was a problem hiding this comment.
This change looks unrelated to comments, the current behavior of scalafmt is to format everything in a single line.
The current behavior of Scalafmt is to never break on select nodes a.b unless b is an "applied" node like a.b() or a.b[T] or a.b { ... }. This is an intentional decision for better or worse, and we're going to change it then we'll need careful performance testing to ensure the search state doesn't explode in cases that formatted fine before.
| @@ -62,7 +62,7 @@ val lastToken = owner.body.tokens.filter { | |||
| case _: Whitespace | _: Comment => false | |||
| case _ => true | |||
| } // edge case, if body is empty expire on arrow. | |||
| .lastOption.getOrElse(arrow) | |||
| .lastOption.getOrElse(arrow) | |||
| val noNewLine = !style.activeForEdition_2019_11 || | ||
| newlines == 0 || rightOwner.isNot[Term.Select] | ||
| val newlinePenalty = if (noNewLine) 0 else 10 + treeDepth(rightOwner) | ||
| Seq( |
There was a problem hiding this comment.
FWIW, doubling the search state here is likely to introduce new "search state exploded" errors for programs that formatted fine before.
Fixes #1334