Add RedundantMap rule #2982
Add RedundantMap rule #2982
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2982 +/- ##
============================================
- Coverage 80.29% 79.78% -0.51%
- Complexity 2478 2515 +37
============================================
Files 424 428 +4
Lines 7479 7553 +74
Branches 1370 1422 +52
============================================
+ Hits 6005 6026 +21
- Misses 760 771 +11
- Partials 714 756 +42 Continue to review full report at Codecov.
|
| """)).isEmpty() | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Could you add a test for list.map { foo -> foo }?
There was a problem hiding this comment.
and another for
list.map {
println("hello")
it
}this map is useless too. If you want to print hello for each value you should use onEach or forEach instead of map.
| /** | ||
| * map { it } does not make sense. You can remove the operator without any impact. | ||
| * <noncompliant> | ||
| * listOf(1, 2).map { it } |
There was a problem hiding this comment.
Are there other examples where just writing it in the lambda expression does not make sense?
Otherwise, the rule seems to be very basic.
There was a problem hiding this comment.
I'm not sure but what about a map that returns a constant? should we flag it?
list.map { "hi" } is the same as List(list.size) { "hi" }... I'm not sure.
Anyway, I found this issues in code after multiple refactors. A map that some time was doing something it ends doing nothing.
| it("should not report filter function") { | ||
| assertThat(RedundantMap().compileAndLint(""" | ||
| fun test(list: List<Boolean>) : List<Boolean> { | ||
| return list.filter { it } | ||
| } | ||
| """)).isEmpty() | ||
| } | ||
|
|
||
| it("should not report filter function") { | ||
| assertThat(RedundantMap().compileAndLint(""" | ||
| fun test(list: List<Boolean>) : List<Boolean> { | ||
| return list.filter { it } | ||
| } | ||
| """)).isEmpty() | ||
| } |
...ules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/RedundantMap.kt
Outdated
Show resolved
Hide resolved
| * </compliant> | ||
| * @since 1.11.0 | ||
| */ | ||
| class RedundantMap( |
There was a problem hiding this comment.
How about RedundantHigherOrderMapUsage ?
| import org.jetbrains.kotlin.psi.KtLambdaExpression | ||
|
|
||
| /** | ||
| * map { it } does not make sense. You can remove the operator without any impact. |
There was a problem hiding this comment.
| * map { it } does not make sense. You can remove the operator without any impact. | |
| * Using map { it } indicates a remnant of a recent refactoring. You can remove the operator without any impact. |
does not make sense is really strong in this context.
| * <compliant> | ||
| * listOf(1, 2).map { it * 2 } | ||
| * </compliant> | ||
| * @since 1.11.0 |
There was a problem hiding this comment.
| * @since 1.11.0 | |
| * @since 1.12.0 |
…etekt/rules/complexity/RedundantMap.kt Co-authored-by: Artur Bosch <arturbosch@gmx.de>
|
I think we should try to get this rule into the next detekt version. It’s a nice addition. Can this merge conflict be handled? |
|
Feature not finished as the moment. Ok I will resolve conflict. |
|
@VovaStelmashchuk no worries. I just thought an up-to-date CI feedback might help. |
|
@schalkmsn @VovaStelmashchuk is this being actively worked on? I'd be happy to take a look if not. |
|
I close this PR. I will come back to you when my solution be ready |
|
@hbmartin please go ahead and feel free to cherry pick the necessary commits. Thank you. 🙂 |
Implement suggest from issue #2975