From 5ff9fbb53b3aaf9e335e26ee4efc83747ad6c8dd Mon Sep 17 00:00:00 2001 From: rileymichael Date: Fri, 23 Aug 2024 08:22:34 -0700 Subject: [PATCH] fix: comment max width (#511) 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 84dbfae449866787ba088d5ea9f0bad48b626cf5 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: https://github.com/facebook/ktfmt/pull/511 Reviewed By: strulovich Differential Revision: D61711870 Pulled By: hick209 fbshipit-source-id: d27479c17f68d376c14722a8d3997ea602cfe723 --- .../facebook/ktfmt/kdoc/KDocCommentsHelper.kt | 8 ++-- .../facebook/ktfmt/format/FormatterTest.kt | 48 +++++++++++++------ .../format/GoogleStyleFormatterKtTest.kt | 42 ++++++++++------ 3 files changed, 65 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt b/core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt index 3cf3c64d..a9450a2a 100644 --- a/core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt +++ b/core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt @@ -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( @@ -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-- diff --git a/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt b/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt index b3b909be..fc9b8b94 100644 --- a/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt @@ -68,7 +68,7 @@ class FormatterTest { fun `call chains`() = assertFormatted( """ - |////////////////////////////////////////////////// + |//////////////////////////////////////////////////////// |fun f() { | // Static method calls are attached to the class name. | ImmutableList.newBuilder() @@ -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() | @@ -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() @@ -765,7 +772,7 @@ class FormatterTest { fun `an assortment of tests for emitQualifiedExpression with lambdas`() = assertFormatted( """ - |//////////////////////////////////////////////////////////////////////////// + |////////////////////////////////////////////////////////////////////////////// |fun f() { | val items = | items.toMutableList.apply { @@ -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 @@ -3134,7 +3141,7 @@ class FormatterTest { fun `properly handle one statement lambda with comment`() = assertFormatted( """ - |/////////////////////// + |//////////////////////// |fun f() { | foo { | // this is a comment @@ -3165,7 +3172,7 @@ class FormatterTest { fun `properly handle one statement lambda with comment after body statements`() = assertFormatted( """ - |/////////////////////// + |//////////////////////// |fun f() { | foo { | red.orange.yellow() @@ -5827,7 +5834,7 @@ class FormatterTest { fun `top level properties with other types preserve newline spacing`() { assertFormatted( """ - |///////////////////////////////// + |///////////////////////////////////////// |fun something() { | println("hi") |} @@ -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 = "\"\"\"" diff --git a/core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt b/core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt index a8078c61..54b717fa 100644 --- a/core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt @@ -157,8 +157,10 @@ 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< @@ -166,7 +168,8 @@ class GoogleStyleFormatterKtTest { | TypeB : Double, | TypeC : String, |> { - | // Type params can't fit on one line + | // Type params can't fit on one + | // line |} | |class Foo< @@ -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 { - | // Class name + type params fit on one line + | // Class name + type params fit on + | // one line |} |""" .trimMargin(), @@ -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 @@ -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() | @@ -1199,25 +1206,29 @@ 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 @@ -1225,7 +1236,8 @@ class GoogleStyleFormatterKtTest { | ) | .thenReturn(result) | - | // Keep 'super' attached to the method name + | // Keep 'super' attached to the + | // method name | super.abcdefghijkl | .methodName4() | .abcdefghijkl()