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.

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

Allow a single whitespace line in chained expressions #1469

Merged
merged 10 commits into from
May 25, 2022
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ An AssertJ style API for testing KtLint rules ([#1444](https://github.com/pinter
- Add experimental rule for consistent spacing before the start of the function body (`function-start-of-body-spacing`) ([#1341](https://github.com/pinterest/ktlint/issues/1341))

### Fixed
- Allow a single whitespace line in chained expressions ([#1469](https://github.com/pinterest/ktlint/pull/1469))
- Fix check of spacing in the receiver type of an anonymous function ([#1440](https://github.com/pinterest/ktlint/issues/1440))
- Allow comment on same line as super class in class declaration `wrapping` ([#1457](https://github.com/pinterest/ktlint/pull/1457))

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package com.pinterest.ktlint.ruleset.standard

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.DOT_QUALIFIED_EXPRESSION
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement

public class NoBlankLinesInChainedMethodCallsRule : Rule("no-blank-lines-in-chained-method-calls") {
override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
val isBlankLine = node is PsiWhiteSpace && node.getText().contains("\n\n")
if (isBlankLine && node.treeParent.elementType == DOT_QUALIFIED_EXPRESSION) {
emit(node.startOffset + 1, "Needless blank line(s)", true)

if (autoCorrect) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we need this check, but I couldn't find a good way to write a test that drives it out. Help would be appreciated!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean if (autoCorrect) { then you have already done so in test case single blank line in dot qualified expression returns lint error.

When calling hasLintViolations, the rule engine is executed with autocorrect set to false which results in emitting the error without fixing it. When calling 'isFormattedAs, the rule engine is invoked with autocorrectset totrue` resulting in formattting the code.

        noBlankLinesInChainedMethodCallsRuleAssertThat(code)
            .hasLintViolations(LintViolation(3, 1, "Needless blank line(s)"))
            .isFormattedAs(
                """
                fun foo(inputText: String) {
                    inputText
                        .toLowerCase()
                }
                """.trimIndent()
            )
            ```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. But, if I remove the if statement (and just always remove the blank line), I don't see a failure. I'm assuming we don't want to be messing around with the code when autocorrect is false. Maybe we need to add a check to hasLintViolations to check that the code is unchanged?

Copy link
Contributor Author

@seadowg seadowg May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paul-dingemans thanks for merging, but I'm still a little concerned there's no coverage for the if (autocorrect) check. I'd be happy to look at that in the future if you agree it's an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think that there is no coverage for the line? As I explained, the line is already covered. You can easily check it out by placing a breakpoint inside the if-statement (line 20) and then run test single blank line in dot qualified expression returns lint error in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, being more specific, I mean that it doesn't have "branch" coverage - it does indeed have "line" coverage. If I change the if statement to just be if (true) for example (or just remove the if and keep the rawReplaceWithText), the tests will still all pass. There's no test that ensures that we only run reformat when autoCorrect is true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to worry about that. There is not a single rule for which this is being tested.

(node as LeafPsiElement).rawReplaceWithText("\n" + node.getText().split("\n\n")[1])
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.pinterest.ktlint.ruleset.standard

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.CLASS
import com.pinterest.ktlint.core.ast.ElementType.DOT_QUALIFIED_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER
import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR
import com.pinterest.ktlint.core.ast.nextLeaf
Expand All @@ -25,20 +24,21 @@ public class NoConsecutiveBlankLinesRule : Rule("no-consecutive-blank-lines") {
if (lfcount < 2) {
return
}

val eof = node.nextLeaf() == null
val prevNode = node.treePrev
val betweenClassAndPrimaryConstructor = prevNode.elementType == IDENTIFIER &&
prevNode.treeParent.elementType == CLASS &&
node.treeNext.elementType == PRIMARY_CONSTRUCTOR
val inDotQualifiedExpression = node.treeParent.elementType == DOT_QUALIFIED_EXPRESSION && lfcount > 1
if (lfcount > 2 || eof || betweenClassAndPrimaryConstructor || inDotQualifiedExpression) {

if (lfcount > 2 || eof || betweenClassAndPrimaryConstructor) {
val split = text.split("\n")
emit(node.startOffset + split[0].length + split[1].length + 2, "Needless blank line(s)", true)
if (autoCorrect) {
val newText = buildString {
append(split.first())
append("\n")
if (!eof && !betweenClassAndPrimaryConstructor && !inDotQualifiedExpression) append("\n")
if (!eof && !betweenClassAndPrimaryConstructor) append("\n")
append(split.last())
}
(node as LeafPsiElement).rawReplaceWithText(newText)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public class StandardRuleSetProvider : RuleSetProvider {
ModifierOrderRule(),
NoBlankLineBeforeRbraceRule(),
NoConsecutiveBlankLinesRule(),
NoBlankLinesInChainedMethodCallsRule(),
NoEmptyClassBodyRule(),
NoLineBreakAfterElseRule(),
NoLineBreakBeforeAssignmentRule(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.pinterest.ktlint.ruleset.standard

import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThat
import com.pinterest.ktlint.test.LintViolation
import org.junit.jupiter.api.Test

class NoBlankLinesInChainedMethodCallsRuleTest {

private val noBlankLinesInChainedMethodCallsRuleAssertThat = NoBlankLinesInChainedMethodCallsRule().assertThat()

@Test
fun `single blank line in dot qualified expression returns lint error`() {
val code =
"""
fun foo(inputText: String) {
inputText

.toLowerCase()
}
""".trimIndent()

noBlankLinesInChainedMethodCallsRuleAssertThat(code)
.hasLintViolations(LintViolation(3, 1, "Needless blank line(s)"))
.isFormattedAs(
"""
fun foo(inputText: String) {
inputText
.toLowerCase()
}
""".trimIndent()
)
}

@Test
fun `single blank line between statements does not return lint error`() {
val code =
"""
fun foo(inputText: String) {
bar()

bar()
}
""".trimIndent()

noBlankLinesInChainedMethodCallsRuleAssertThat(code).hasNoLintViolations()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package com.pinterest.ktlint.ruleset.standard
import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThat
import com.pinterest.ktlint.test.LintViolation
import com.pinterest.ktlint.test.MULTILINE_STRING_QUOTE
import com.pinterest.ktlint.test.format
import com.pinterest.ktlint.test.lint
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Nested
Expand Down Expand Up @@ -176,25 +175,41 @@ class NoConsecutiveBlankLinesRuleTest {
}

@Test
fun `should remove line in dot qualified expression`() {
assertThat(
NoConsecutiveBlankLinesRule().format(
"""
fun foo(inputText: String) {
inputText
fun `Given single blank line in dot qualified expression should not return lint errors`() {
val code =
"""
fun foo(inputText: String) {
inputText

.toLowerCase()
}
""".trimIndent()

.toLowerCase()
}
""".trimIndent()
)
).isEqualTo(
noConsecutiveBlankLinesRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Given multiple blank line in dot qualified expression should return lint error`() {
val code =
"""
fun foo(inputText: String) {
inputText


.toLowerCase()
}
""".trimIndent()
)

noConsecutiveBlankLinesRuleAssertThat(code)
.hasLintViolations(LintViolation(4, 1, "Needless blank line(s)"))
.isFormattedAs(
"""
fun foo(inputText: String) {
inputText

.toLowerCase()
}
""".trimIndent()
)
}
}