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

Add CascadingCallWrapping style rule #4979

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Add CascadingCallWrapping style rule #4979

merged 1 commit into from
Jun 22, 2022

Conversation

dzirbel
Copy link
Contributor

@dzirbel dzirbel commented Jun 21, 2022

Add a new rule CascadingCallWrapping which requires that if a chained call is placed on a newline then all subsequent calls must be as well, improving readability of long chains.

The rule name was chosen to be less ambiguous with ktlint's ChainWrapping, but could possibly be improved. I've taken the liberty of enabling it in detekt's own configuration file and fix a handful of existing issues.

@3flex
Copy link
Member

3flex commented Jun 21, 2022

Thank you for this rule! I've been wanting this for a while but didn't get around to writing it.

I'll review in more detail later. One thought: would you consider adding a configuration where there is a maximum number of chained calls on a single line? E.g. allow only 2 on a single line as a maximum, then require wrapping?

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 21, 2022

One thought: would you consider adding a configuration where there is a maximum number of chained calls on a single line? E.g. allow only 2 on a single line as a maximum, then require wrapping?

I'd had the same thought but didn't add it initially since I thought it might add too much complication. Happy to add it here, but it raises the question of how it would be configured, e.g. 0 to allow any number of calls on the first line? An alternative is to enforce that as a separate rule, which could then be more intuitively configured. Thoughts?

Another piece I was thinking about is whether to require wrapping for any calls that are split across multiple lines; there's a test case for the current behavior but personally I'd tend to wrap each of those calls. I was also leaning toward enforcing that in a different rule to avoid making this one too heavy but it also has a natural connection.

Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

This is great! I'm a big fan of this rule, thanks again for implementing.

@3flex
Copy link
Member

3flex commented Jun 21, 2022

Good point on the configuration concerns, I hadn't thought that far ahead. A separate rule probably makes sense - if it just checks if there are x chained calls and that's over the y threshold then it would report a violation and tell you to split over multiple lines. Then this rule would still be there to make sure that if the chain calls are split, then all of them are on a unique line. That makes sense.

whether to require wrapping for any calls that are split across multiple lines; there's a test case for the current behavior but personally I'd tend to wrap each of those calls

I think that behaviour is fine and reflects this semi-official guidance: Kotlin/kotlin-style-guide#9 (comment)

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 21, 2022

I think that behaviour is fine and reflects this semi-official guidance: Kotlin/kotlin-style-guide#9 (comment)

Thanks for the link - didn't realize there was a project for contribution to the style guide! I think using breaks probably depends on the number of chained calls but IMO it's still subjective at best, so I'm also leaning toward not adding it as a rule.

Comment on lines 23 to 30
assertThat(findings).hasSize(1)

val finding = findings.first()
assertEquals(TextLocation(start = 8, end = 30), finding.charPosition)
assertEquals(
"Chained call `plus(0)` should be wrapped to a new line since preceding calls were.",
finding.message,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use junit assertions

Suggested change
assertThat(findings).hasSize(1)
val finding = findings.first()
assertEquals(TextLocation(start = 8, end = 30), finding.charPosition)
assertEquals(
"Chained call `plus(0)` should be wrapped to a new line since preceding calls were.",
finding.message,
)
assertThat(findings)
.hasSize(1)
.hasTextLocations(8 to 30)
assertThat(findings.first())
.hasMessage("Chained call `plus(0)` should be wrapped to a new line since preceding calls were.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I'll do you one better by using first() on the assertion

}
}

@Suppress("CanBeNonNullable") // false positive: callExpression cannot be made non-nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with detekt here ;) In which circumstances would the callExpression actually be null?

If the callExpresssion actually is null, this will mess up the error message. IMO it would be better to verify on the call side that the parameter is actually non null.

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 think this is a clear false positive; callExpression can't just be made non-nullable - it can be null when either KtQualifiedExpression.selectorExpression or KtBinaryExpression.right are null (which I've never seen in practice, but they are both declared nullable), and the function isn't a no-op when it is null.

But your point still stands that the error message will be less useful if it is null. I don't think the solution is to verify it on the call side since I think we'll still want to generate a warning even if the right expression is missing. I'll improve the error message for the null case to just omit it.

Comment on lines 111 to 119
private fun String.containsInRange(needle: Char, startIndex: Int, endIndex: Int): Boolean {
for (i in startIndex until endIndex) {
if (this[i] == needle) {
return true
}
}

return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

    private fun String.containsInRange(needle: Char, range: IntRange): Boolean {
        return range.any { this[it] == needle }
    }

or

    private fun String.containsInRange(needle: Char, startIndex: Int, endIndex: Int): Boolean {
        return (startIndex until endIndex).any { this[it] == needle }
    }

Feel free to ignore this comment ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea; this was probably a premature optimization to start with but using your second idea at the call site (since it's only used once) makes it cleaner.

Add a new rule CascadingCallWrapping which requires that if a chained call is placed on a newline then all subsequent calls must be as well, improving readability of long chains.
@cortinico cortinico added this to the 1.21.0 milestone Jun 22, 2022
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Jun 22, 2022
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Nice rule :) Thanks for sending this over

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

👏👏👏 Great rule 👏👏👏

@BraisGabin BraisGabin merged commit 89f6ec1 into detekt:main Jun 22, 2022
@dzirbel dzirbel deleted the allChainedCallsOnNewLine branch June 22, 2022 20:12
BraisGabin pushed a commit that referenced this pull request Jun 28, 2022
Add a new rule MaxChainedCallsOnSameLine to limit the number of chained calls on placed on a single line. This works well alongside CascadingCallWrapping in #4979 to make long call chains more readable by wrapping them on new lines.
@DHosseiny
Copy link

DHosseiny commented Nov 30, 2022

This is a useful rule, thanks for writing this. But there is a case this is annoying. Example:

tabsToDeepLinks
    .toList()
    .firstOrNull { pair ->
        pair.second == toUri
    }?.let { pair ->
        switchTab(tabIndex = pair.first)
        return true
    }

The let function is after a lambda so in this case, I think it is acceptable to not be in a new line. Generally speaking, let the chained call be in the same line with the previous function's lambda end if the previous function's lambda is not one lined. At least add a Configuration options for this case.

@DHosseiny
Copy link

Let me know if I should open an issue for this.

@BraisGabin
Copy link
Member

I don't agree with that but please, open an issue so other people can give their opinions

@DHosseiny
Copy link

#5587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api gradle-plugin notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants