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

Jump to bottom

fix: comment max width #511

Closed
wants to merge 3 commits into from
Closed

fix: comment max width #511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd4d61b
add failing comment maxWidth test
rileymichael Aug 20, 2024
1839ec2
use maxLineLength for line comments
rileymichael Aug 20, 2024
84dbfae
fix tests that deduced max width
rileymichael Aug 20, 2024
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
8 changes: 4 additions & 4 deletions core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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