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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
- Move disallowing blank lines in chained method calls from `no-consecutive-blank-lines` to new rule (`no-blank-lines-in-chained-method-calls`) ([#1248](https://github.com/pinterest/ktlint/issues/1248))
- 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ It's also [easy to create your own](#creating-a-reporter).
- `max-line-length`: Ensures that lines do not exceed the given length of `.editorconfig` property `max_line_length` (see [EditorConfig](#editorconfig) section for more). This rule does not apply in a number of situations. For example, in the case a line exceeds the maximum line length due to and comment that disables ktlint rules than that comment is being ignored when validating the length of the line. The `.editorconfig` property `ktlint_ignore_back_ticked_identifier` can be set to ignore identifiers which are enclosed in backticks, which for example is very useful when you want to allow longer names for unit tests.
- `modifier-order`: Consistent order of modifiers
- `no-blank-line-before-rbrace`: No blank lines before `}`
- `no-blank-lines-in-chained-method-calls`: No blank lines in chained method expressions
- `no-consecutive-blank-lines`: No consecutive blank lines
- `no-empty-class-body`: No empty (`{}`) class bodies
- `no-line-break-after-else`: Disallows line breaks after the else keyword if that could lead to confusion, for example:
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,43 @@
package com.pinterest.ktlint.ruleset.standard

import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThat
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()
val formattedCode =
"""
fun foo(inputText: String) {
inputText
.toLowerCase()
}
""".trimIndent()
noBlankLinesInChainedMethodCallsRuleAssertThat(code)
.hasLintViolation(3, 1, "Needless blank line(s)")
.isFormattedAs(formattedCode)
}

@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()
)
}
}