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

Allow a single whitespace line in chained expressions #1469

Merged
merged 10 commits into from
May 25, 2022
Merged

Allow a single whitespace line in chained expressions #1469

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9d3024c
Allow one whitespace line in chained expressions
seadowg May 16, 2022
cc21285
Update CHANGELOG
seadowg May 16, 2022
4cd2566
Add new empty rule to standard set
seadowg May 23, 2022
4828472
Don't allow blank lines in chained method calls
seadowg May 23, 2022
7a00b36
Remove blank line from chained methods
seadowg May 23, 2022
afd6846
Only format when autoCorrect is true
seadowg May 23, 2022
ae7202f
Add new rule to README
seadowg May 23, 2022
2eb7c87
Update changelog
seadowg May 23, 2022
aefb60c
Refactor test
May 24, 2022
5fb5428
Fix changelog and readme
May 24, 2022
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
5 changes: 4 additions & 1 deletion ...main/kotlin/com/pinterest/ktlint/ruleset/standard/NoBlankLinesInChainedMethodCallsRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ public class NoBlankLinesInChainedMethodCallsRule : Rule("no-blank-lines-in-chai
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)
(node as LeafPsiElement).rawReplaceWithText("\n" + node.getText().split("\n\n")[1])

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 •
edited
Loading

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])
}
}
}
}