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 MaxChainedCallsOnSameLine style rule #4985

Merged
merged 1 commit into from Jun 28, 2022
Merged

Add MaxChainedCallsOnSameLine style rule #4985

merged 1 commit into from Jun 28, 2022

Conversation

dzirbel
Copy link
Contributor

@dzirbel dzirbel commented Jun 21, 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.

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.
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Jun 22, 2022
@cortinico cortinico added this to the 1.21.0 milestone Jun 22, 2022
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.

What do you think about ?: should it count? I can imagine a chain of elvis operators too... I'm not 100% sure about this, I'm just asking for more opinions.

)

@Configuration("maximum chained calls allowed on a single line")
private val maxChainedCalls: Int by config(defaultValue = 5)
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Isn't 5 far too much? I would say 3 or 4. But this is really subjective.

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 was leaning toward making it more conservative, since in my experience it's not uncommon to have calls on e.g. network models that can be at least a few references deep. But I don't feel strongly; happy to change to any other value.

@BraisGabin
Copy link
Member

Again, great rule :)

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 22, 2022

What do you think about ?: should it count? I can imagine a chain of elvis operators too... I'm not 100% sure about this, I'm just asking for more opinions.

I agree in practice it contributes to the complexity of a line in the same way; I didn't include just since it seemed more likely to be unintuitive. Also more than happy to add it if others have thoughts, or include it as a configuration parameter (but that's likely to overcomplicate it as well).

@BraisGabin
Copy link
Member

I approve this PR but I would like to know what others think about the default value. I feel it should be lowered a bit but I would like to have more opinions here.

@cortinico
Copy link
Member

I feel it should be lowered a bit but I would like to have more opinions here.

I think 5 is a good number. Don't have a strong opinion. As the rule is disabled by default, users can easily customize it once they enable it

@BraisGabin BraisGabin merged commit 1e696fd into detekt:main Jun 28, 2022
@dzirbel dzirbel deleted the maxChainedCallsOnSameLine branch July 25, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants