Consecutive blank lines are reported in chained method calls

[mindhaq]

Expected Behavior

Comments within chained method calls should be able to have a new line before them.

Observed Behavior

The line with the comment is marked as problematic

 Needless blank line(s) (no-consecutive-blank-lines)

Steps to Reproduce

Example:

 // when
 webTestClient
    .get()
    .uri(requestUri)
    .exchange()

    // then
    .expectStatus().isNotFound

Your Environment

• Version of ktlint used: 0.42.1
• Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): Gradle with org.jlleitschuh.gradle.ktlint:10.2.0
• Version of Gradle used (if applicable): 7.2
• Operating System and version: Mac OS / Linux

[paul-dingemans]

This seems to be a personal preference to me. If you can find any explicit advice on this in the Kotlin style guide, please support a quote and link to it.

Maybe you can work around it as follows:

 // when
 webTestClient
    .get()
    .uri(requestUri)
    .exchange()
    //
    // then
    .expectStatus().isNotFound

Otherwise, you can disable the rule.

[seadowg]

Can completely see how this could be regarded as a preference, but I'm confused as to why ktlint is treating a comment line as a "blank line" here (as in the original example). I definitely have cases (mainly in tests) where it's useful to break up a chain with a blank line and a comment, and it'd be nice to be able to do that without adding a blank comment (which looks strange) or disabling the rule everywhere.

[grzesiek2010]

I also don't like this change but at the same time I don't want to disable the whole rule no-consecutive-blank-lines.

[mindhaq]

The problem I have is

1. My code was perfectly accepted with previous versions, this seems to have started with 0.42.0.
2. I do not have consecutive blank lines, I only have one blank line. Single blank lines should be allowed in chained operations, as they are in many other places as well.

[paul-dingemans]

I do understand your arguments. However, we do not want to bloat ktlint with configuration options to tune behavior within rules. So in cases like this, a choice has to be made. In this case the generic rule to disallow consecutive blank lines makes more sense than allowing it when it make sense in a specific case.

[mindhaq]

I fully agree that there should not be consecutive blank lines when this rule is applied.

But I only have one single blank line, and it is reported as needless.

Or would you say that new lines in chained method calls should not be allowed at all with this rule in place? Then there should maybe be a rule called no-blank-lines-in-method-chains?

[seadowg]

@paul-dingemans agreed it shouldn't be added as a special case that chains support multiple consecutive blank lines. As @mindhaq points out it's that we're arguing his example only has a single blank line in this section:

.exchange()

// then
.expectStatus().isNotFound

I initially thought that ktlint was treating the comment as blank but actually, it seems like this is fine:

doSomething()

// explain the line below
doSomethingElse()

...but this fails:

startChain()
    .doAThing()

    .doAnotherThing()

So for me, the issue isn't that we want a special case for consecutive blank lines in chains it's that it seems (to me) that ktlint is incorrectly identifying consecutive blank lines in chains. It'd be great if someone could double-check my findings here in case I've messed them up 😆

[paul-dingemans]

I see your point now. The rule is called "consecutive-blank-lines". It already contained functionality for reporting needless blank lines before I expanded it to chained call. That functionality regarding needless blank lines should not have been integrated in this rule as it does not match with what you expect based on te rule name.

Suppose that the needless blank line detection was moved to a separate rule then I would still argue that the blank line has to be removed from the chained calls. So you would still end up with the same problem unless you would disable that rule. But then you would still loose functionality about fixing other needless blank lines.

[seadowg]

That functionality regarding needless blank lines should not have been integrated in this rule as it does not match with what you expect based on te rule name.

👍 That sounds right to me! I would definitely be in favour of separating out "needless" from "consecutive" or at least just renaming the rule to needless-blank-lines to make it clearer what's going on.

Suppose that the needless blank line detection was moved to a separate rule then I would still argue that the blank line has to be removed from the chained calls.

I don't think (unless I've missed it) there's precedent for not having blank lines in chained calls in either the Kotlin coding conventions or the Android Kotlin style guide. In fact, I think you could argue that the Android style guide, which it seems the standard Kotlin rules for vertical whitespace in ktlint are based on given kotlinlang.org doesn't state any, covers the case described in the issue with blank lines being encouraged "Between statements, as needed to organize the code into logical subsections.".

[paul-dingemans]

Lol, except that it states 'between statements'. A chained call is a single statement in my opinion.

[seadowg]

A chained call is a single statement in my opinion.

😆 hah yes that's a fair point. I guess it's a weird one where fluent APIs (when combined with Page Objects as an example) can result in a stranger style where you might have separate logical blocks of calls, but yes I agree, it's all technically one "statement" and hard to justify the blank line against either style guide.

[paul-dingemans]

I would definitely be in favour of separating out "needless" from "consecutive" or at least just renaming the rule to needless-blank-lines to make it clearer what's going on.

@romtsn @Tapchicoma How do you think about renaming an existing rule to match the name with intent? Or would you prefer splitting the rules? Renaming is less work but might confuse users who have this specific rule disabled.

[romtsn]

Am actually not sure if the fix should've been implemented in the first place.. when I toss that code snippet from the original issue (#1077) into IJ formatted, it just leaves it unchanged - so would I.

[paul-dingemans]

Now I am getting confused again. If the outcome of ktlint has to match with formatting of IntelliJ IDEA always, what value does ktlint then provide? In that case, You can use IntelliJ auto formatter only.

The ktlint rules should improve the code. Outcome of the rules, should not result in code that is 'disapproved' by IntelliJ. If both those conditions are met, a rule provides value.

Note that consecutive blank lines are also allowed by the editor.

[cyber-barrista]

Is there any update on this? I'd vote for the splitting. These "needless" line breaks in chained calls come in handy in tests/builders as a matter of splitting semantically segregated calls. Now, this handiness can be achieved only at the cost of losing no-consecutive-breaks validation which is sad.

[paul-dingemans]

Is there any update on this? I'd vote for the splitting. These "needless" line breaks in chained calls come in handy in tests/builders as a matter of splitting semantically segregated calls. Now, this handiness can be achieved only at the cost of losing no-consecutive-breaks validation which is sad.

No, sorry no update. This issue is cluttered with different opinions and unanswered questions. As long as there is no alignment on what we thinkt is best, there will no progress. It might help to draft down one clear proposal where no functionality is lost but which is still achievable. Or be bold and create a PR ;-)