Skip to content

Commit

Permalink
fix: comment max width (#511)
Browse files Browse the repository at this point in the history 
Summary:
`FormattingOptions#maxWidth` is not respected for double slash comments. This is because it wasn't using the supplied value, and instead checked against the static property `com.google.googlejavaformat.java.Formatter.MAX_LINE_LENGTH`. I was unable to find any documentation / discussion that would indicate this as being intentional.

Test cases that deduced `maxWidth` actually relied on this behavior in some cases. Some of the fixes in 84dbfae simply adjusted the number of `/`, while others relied on a specific `maxWidth` and so I manually line-broke the comments to preserve the behavior under test.

Pull Request resolved: #511

Reviewed By: strulovich

Differential Revision: D61711870

Pulled By: hick209

fbshipit-source-id: d27479c17f68d376c14722a8d3997ea602cfe723
  • Loading branch information
@rileymichael @facebook-github-bot
rileymichael authored and facebook-github-bot committed Aug 23, 2024
1 parent 6b7ca01 commit 5ff9fbb
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 33 deletions.
8 changes: 4 additions & 4 deletions core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import com.google.common.base.Strings
import com.google.googlejavaformat.CommentsHelper
import com.google.googlejavaformat.Input.Tok
import com.google.googlejavaformat.Newlines
import com.google.googlejavaformat.java.Formatter
import java.util.ArrayList
import java.util.regex.Pattern

/** `KDocCommentsHelper` extends [CommentsHelper] to rewrite KDoc comments. */
class KDocCommentsHelper(private val lineSeparator: String, maxLineLength: Int) : CommentsHelper {
class KDocCommentsHelper(private val lineSeparator: String, private val maxLineLength: Int) :
CommentsHelper {

private val kdocFormatter =
KDocFormatter(
Expand Down Expand Up @@ -119,8 +119,8 @@ class KDocCommentsHelper(private val lineSeparator: String, maxLineLength: Int)
result.add(line)
continue
}
while (line.length + column0 > Formatter.MAX_LINE_LENGTH) {
var idx = Formatter.MAX_LINE_LENGTH - column0
while (line.length + column0 > maxLineLength) {
var idx = maxLineLength - column0
// only break on whitespace characters, and ignore the leading `// `
while (idx >= 2 && !CharMatcher.whitespace().matches(line[idx])) {
idx--
Expand Down
48 changes: 34 additions & 14 deletions core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class FormatterTest {
fun `call chains`() =
assertFormatted(
"""
|//////////////////////////////////////////////////
|////////////////////////////////////////////////////////
|fun f() {
| // Static method calls are attached to the class name.
| ImmutableList.newBuilder()
Expand Down Expand Up @@ -715,12 +715,14 @@ class FormatterTest {
"""
|/////////////////////////////////////
|fun f() {
| // Regression test: https://github.com/facebook/ktfmt/issues/56
| // Regression test:
| // https://github.com/facebook/ktfmt/issues/56
| kjsdfglkjdfgkjdfkgjhkerjghkdfj
| ?.methodName1()
|
| // a series of field accesses followed by a single call expression
| // is kept together.
| // a series of field accesses
| // followed by a single call
| // expression is kept together.
| abcdefghijkl.abcdefghijkl
| ?.methodName2()
|
Expand All @@ -729,30 +731,35 @@ class FormatterTest {
| ?.methodName3
| ?.abcdefghijkl()
|
| // Multiple call expressions cause each part of the expression
| // Multiple call expressions cause
| // each part of the expression
| // to be placed on its own line.
| abcdefghijkl
| ?.abcdefghijkl
| ?.methodName4()
| ?.abcdefghijkl()
|
| // Don't break first call expression if it fits.
| // Don't break first call
| // expression if it fits.
| foIt(something.something.happens())
| .thenReturn(result)
|
| // Break after `longerThanFour(` because it's longer than 4 chars
| // Break after `longerThanFour(`
| // because it's longer than 4 chars
| longerThanFour(
| something.something
| .happens())
| .thenReturn(result)
|
| // Similarly to above, when part of qualified expression.
| // Similarly to above, when part of
| // qualified expression.
| foo.longerThanFour(
| something.something
| .happens())
| .thenReturn(result)
|
| // Keep 'super' attached to the method name
| // Keep 'super' attached to the
| // method name
| super.abcdefghijkl
| .methodName4()
| .abcdefghijkl()
Expand All @@ -765,7 +772,7 @@ class FormatterTest {
fun `an assortment of tests for emitQualifiedExpression with lambdas`() =
assertFormatted(
"""
|////////////////////////////////////////////////////////////////////////////
|//////////////////////////////////////////////////////////////////////////////
|fun f() {
| val items =
| items.toMutableList.apply {
Expand Down Expand Up @@ -865,7 +872,7 @@ class FormatterTest {
fun `don't one-line lambdas following argument breaks`() =
assertFormatted(
"""
|////////////////////////////////////////////////////////////////////////
|///////////////////////////////////////////////////////////////////////////////
|class Foo : Bar() {
| fun doIt() {
| // don't break in lambda, no argument breaks found
Expand Down Expand Up @@ -3134,7 +3141,7 @@ class FormatterTest {
fun `properly handle one statement lambda with comment`() =
assertFormatted(
"""
|///////////////////////
|////////////////////////
|fun f() {
| foo {
| // this is a comment
Expand Down Expand Up @@ -3165,7 +3172,7 @@ class FormatterTest {
fun `properly handle one statement lambda with comment after body statements`() =
assertFormatted(
"""
|///////////////////////
|////////////////////////
|fun f() {
| foo {
| red.orange.yellow()
Expand Down Expand Up @@ -5827,7 +5834,7 @@ class FormatterTest {
fun `top level properties with other types preserve newline spacing`() {
assertFormatted(
"""
|/////////////////////////////////
|/////////////////////////////////////////
|fun something() {
| println("hi")
|}
Expand Down Expand Up @@ -7665,6 +7672,19 @@ class FormatterTest {
assertFormatted(third)
}

@Test
fun `comment formatting respects max width`() {
val code =
"""
|// This is a very long comment that is very long but does not need to be line broken as it is within maxWidth
|class MyClass {}
|"""
.trimMargin()
assertThatFormatting(code)
.withOptions(defaultTestFormattingOptions.copy(maxWidth = 120))
.isEqualTo(code)
}

companion object {
/** Triple quotes, useful to use within triple-quoted strings. */
private const val TQ = "\"\"\""
Expand Down
42 changes: 27 additions & 15 deletions core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,16 +157,19 @@ class GoogleStyleFormatterKtTest {
| TypeA : Int,
| TypeC : String,
|> {
| // Class name + type params too long for one line
| // Type params could fit on one line but break
| // Class name + type params too
| // long for one line
| // Type params could fit on one
| // line but break
|}
|
|class Foo<
| TypeA : Int,
| TypeB : Double,
| TypeC : String,
|> {
| // Type params can't fit on one line
| // Type params can't fit on one
| // line
|}
|
|class Foo<
Expand All @@ -188,12 +191,14 @@ class GoogleStyleFormatterKtTest {
| TypeB : Double,
| TypeC : String,
|>(a: Int, var b: Int, val c: Int) {
| // TODO: Breaking the type param list
| // should propagate to the value param list
| // TODO: Breaking the type param
| // list should propagate to the
| // value param list
|}
|
|class C<A : Int, B : Int, C : Int> {
| // Class name + type params fit on one line
| // Class name + type params fit on
| // one line
|}
|"""
.trimMargin(),
Expand Down Expand Up @@ -418,7 +423,7 @@ class GoogleStyleFormatterKtTest {
fun `don't one-line lambdas following argument breaks`() =
assertFormatted(
"""
|////////////////////////////////////////////////////////////////////////
|///////////////////////////////////////////////////////////////////////////////
|class Foo : Bar() {
| fun doIt() {
| // don't break in lambda, no argument breaks found
Expand Down Expand Up @@ -1185,12 +1190,14 @@ class GoogleStyleFormatterKtTest {
"""
|//////////////////////////////////////
|fun f() {
| // Regression test: https://github.com/facebook/ktfmt/issues/56
| // Regression test:
| // https://github.com/facebook/ktfmt/issues/56
| kjsdfglkjdfgkjdfkgjhkerjghkdfj
| ?.methodName1()
|
| // a series of field accesses followed by a single call expression
| // is kept together.
| // a series of field accesses
| // followed by a single call
| // expression is kept together.
| abcdefghijkl.abcdefghijkl
| ?.methodName2()
|
Expand All @@ -1199,33 +1206,38 @@ class GoogleStyleFormatterKtTest {
| ?.methodName3
| ?.abcdefghijkl()
|
| // Multiple call expressions cause each part of the expression
| // Multiple call expressions cause
| // each part of the expression
| // to be placed on its own line.
| abcdefghijkl
| ?.abcdefghijkl
| ?.methodName4()
| ?.abcdefghijkl()
|
| // Don't break first call expression if it fits.
| // Don't break first call
| // expression if it fits.
| foIt(something.something.happens())
| .thenReturn(result)
|
| // Break after `longerThanFour(` because it's longer than 4 chars
| // Break after `longerThanFour(`
| // because it's longer than 4 chars
| longerThanFour(
| something.somethingBlaBla
| .happens()
| )
| .thenReturn(result)
|
| // Similarly to above, when part of qualified expression.
| // Similarly to above, when part of
| // qualified expression.
| foo
| .longerThanFour(
| something.somethingBlaBla
| .happens()
| )
| .thenReturn(result)
|
| // Keep 'super' attached to the method name
| // Keep 'super' attached to the
| // method name
| super.abcdefghijkl
| .methodName4()
| .abcdefghijkl()
Expand Down

0 comments on commit 5ff9fbb

Please sign in to comment.