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

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 detekt#4979 to make long call chains more readable by wrapping them on new lines.
@dzirbel dzirbel force-pushed the maxChainedCallsOnSameLine branch from 8854f08 to 0acd9b6 Compare Jun 21, 2022
@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
Collaborator

@BraisGabin BraisGabin left a comment

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
Collaborator

@BraisGabin BraisGabin Jun 22, 2022

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

@dzirbel dzirbel Jun 22, 2022

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
Collaborator

BraisGabin commented Jun 22, 2022

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
Collaborator

BraisGabin commented Jun 27, 2022

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

cortinico commented Jun 28, 2022

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
19 checks passed
@dzirbel dzirbel deleted the maxChainedCallsOnSameLine branch Jul 25, 2022
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