Allow a single whitespace line in chained expressions

[seadowg]

Description

Closes #1248

There's a lot of discussion in the issues as to the right approach, but here's my proposal for a fix. Given the rule is intended to detect "consecutive" blank lines, I've made sure a single blank line is allowed in a chained expression (but that multiple is still disallowed and formats correctly). This will allow for the "blank line followed by a comment" examples brought up in the issue and keeps the whitespace rules consistent between regular statements and chained expressions.

The reasoning behind arguing for blank lines to be allowed in chained expressions is that there's nothing about disallowing them in either the Kotlin coding conventions or the Android Kotlin style guide (as far as I can see) and people have brought up use cases where it's useful in the issue.

Checklist

• PR description added
• tests are added
• CHANGELOG.md is updated

[paul-dingemans]

@seadowg Tnx for making the effort for this PR.

Basically you have reverted issue #1105 without offering an alternative solution for that issue. I do not think that we should do so as that functionality is expected to be in place by other users who do not want allow single blank line within dot qualified expression.

So I think that your PR has to be expanded with a new rule that just disallows (needless) blank lines in a dot qualified expression. This new rule should be a simplified version of the current rule. To allow blank lines in a dot qualified expression you will need to disable that additional rule in your projects.

[seadowg]

So I think that your PR has to be expanded with a new rule that just disallows (needless) blank lines in a dot qualified expression. This new rule should be a simplified version of the current rule. To allow blank lines in a dot qualified expression you will need to disable that additional rule in your projects.

Sounds good! For the name of the new rule, how about no-blank-lines-in-chain? That's what pops into my head after looking at the language used in chain-wrapping and no-consecutive-blank-lines.

[paul-dingemans]

Sounds good! For the name of the new rule, how about no-blank-lines-in-chain? That's what pops into my head after looking at the language used in chain-wrapping and no-consecutive-blank-lines.

I prefer the longer version no-blank-lines-in-chained-method-calls.

[seadowg]

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!

[paul-dingemans]

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()
            )
            ```

[seadowg]

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?

[seadowg]

@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?

[paul-dingemans]

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.

[seadowg]

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.

[paul-dingemans]

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

[seadowg]

@paul-dingemans this is probably ready for another look.

[paul-dingemans]

Tnx for your contribution @seadowg. I hope you like contributing to ktlint. Please feel free to pick up another issue from the list!