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 MultilineLambdaItParameter rule #3259

Merged
merged 5 commits into from
Jan 14, 2021

Conversation

adamkobor
Copy link
Contributor

This new rule checks if there is any multiline lambda with a parameter called it (whether it's an implicit or an explicit one, it doesn't matter).

@codecov
Copy link

codecov bot commented Nov 29, 2020

Codecov Report

Merging #3259 (3e924d5) into master (d3a3734) will decrease coverage by 0.05%.
The diff coverage is 79.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3259      +/-   ##
============================================
- Coverage     80.34%   80.29%   -0.06%     
+ Complexity     2724     2723       -1     
============================================
  Files           445      446       +1     
  Lines          8177     8251      +74     
  Branches       1555     1565      +10     
============================================
+ Hits           6570     6625      +55     
- Misses          774      785      +11     
- Partials        833      841       +8     
Impacted Files Coverage Δ Complexity Δ
...n/kotlin/io/gitlab/arturbosch/detekt/api/Config.kt 100.00% <ø> (ø) 0.00 <0.00> (ø)
...lin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt 66.66% <ø> (ø) 0.00 <0.00> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/api/Context.kt 70.00% <0.00%> (+6.36%) 0.00 <0.00> (ø)
...kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt 62.50% <0.00%> (-8.93%) 0.00 <0.00> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt 70.00% <0.00%> (+6.36%) 3.00 <1.00> (-1.00) ⬆️
...h/detekt/core/baseline/IndentingXMLStreamWriter.kt 58.82% <ø> (-1.56%) 14.00 <0.00> (-1.00)
.../arturbosch/detekt/core/tooling/ParsingStrategy.kt 33.33% <0.00%> (+6.06%) 0.00 <0.00> (ø)
...enerator/printer/rulesetpage/RuleSetPagePrinter.kt 86.84% <ø> (-0.97%) 16.00 <0.00> (-2.00)
...tlab/arturbosch/detekt/DetektCreateBaselineTask.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...tlab/arturbosch/detekt/extensions/DetektReports.kt 41.17% <ø> (ø) 6.00 <0.00> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3a3734...8c340d2. Read the comment docs.

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.

Thanks for your contribution @adamkobor.
I'd love to hear other members feedback on the idea behind your rule. I'm also wondering if we should merge the behavior inside the ExplicitItLambdaParameter rule and treat this as a config option.

@adamkobor
Copy link
Contributor Author

Thanks for your contribution @adamkobor.
I'd love to hear other members feedback on the idea behind your rule. I'm also wondering if we should merge the behavior inside the ExplicitItLambdaParameter rule and treat this as a config option.

Thanks! Initially, I was thinking about the same (extending ExplicitItLambdaParameter somehow), but it seems like a quite different rule for me, speaking of what it wants to enforce. Even if we could merge the two, it would be very hard to find a good name for the new rule IMO :)
However, I'm open to any kind of suggestion about this if you have a better idea 🙌

@BraisGabin
Copy link
Member

Nice rule!

I think that they are different rules. We need new buckets to define rules because this one helps readability but the other just removes redundant code.

I would keep this one as a different rule.

And should we care if the code defines the parameter as it? It's just an open question. I have no idea which one is the bette option.

Comment on lines +51 to +55
* val digits = 1234.let { it -> listOf(it) }
* val digits = 1234.let { it ->
* listOf(it)
* }
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

IT_LITERAL in parameterNames is already checked in the rule mentioned above, so we don't need to double report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I totally agree that these rules actually have some overlap, but I think their perspective are quite different. Therefore, I wouldn't remove this overlap, because it's possible that someone is using only one of them. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

People may enable both rules, and they may get double issues...

That could be our expectation since abusing it is lowering the readability significantly.
I am okay with keeping it as is.

Copy link
Member

Choose a reason for hiding this comment

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

People may enable both rules, and they may get double issues...

True, that's a scenario that will lead to a double reporting:

  val digits = 1234.let { it ->
    println(it)
    listOf(it)
  }

(i.e. multiline lambdas + explicit it). I'm anyway fine with it as the only way to resolve this is to have an explicit lambda parameter name.

So I'm still in for merging this rule 👌

@adamkobor
Copy link
Contributor Author

Sorry for the late answer, I will look into this in the coming days, but we have family issues right now at home with our newborn, so my time is quite limited :)

@adamkobor adamkobor force-pushed the multiline-lambda-it-param-rule branch from c5927ea to b2720e6 Compare December 30, 2020 12:16
@adamkobor adamkobor requested a review from cortinico December 30, 2020 12:29
@cortinico
Copy link
Member

but we have family issues right now at home with our newborn, so my time is quite limited :)

Sure take your time :) If you need some help just drop us a message

@adamkobor
Copy link
Contributor Author

but we have family issues right now at home with our newborn, so my time is quite limited :)

Sure take your time :) If you need some help just drop us a message

Thanks @cortinico, I already made the adjustments you and @chao2zhang requested, so if you have some time, please take a look at it :)

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.

Other than minor comments, LGTM 👍

Comment on lines +51 to +55
* val digits = 1234.let { it -> listOf(it) }
* val digits = 1234.let { it ->
* listOf(it)
* }
Copy link
Member

Choose a reason for hiding this comment

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

People may enable both rules, and they may get double issues...

True, that's a scenario that will lead to a double reporting:

  val digits = 1234.let { it ->
    println(it)
    listOf(it)
  }

(i.e. multiline lambdas + explicit it). I'm anyway fine with it as the only way to resolve this is to have an explicit lambda parameter name.

So I'm still in for merging this rule 👌

Co-authored-by: Nicola Corti <corti.nico@gmail.com>
@adamkobor
Copy link
Contributor Author

Other than minor comments, LGTM 👍

Thanks, I also applied your suggestions 🙌

@schalkms schalkms merged commit a17b7f9 into detekt:master Jan 14, 2021
@arturbosch arturbosch added this to the 1.16.0 milestone Jan 18, 2021
@adamkobor adamkobor deleted the multiline-lambda-it-param-rule branch January 27, 2021 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants