Cleanup code #2862
Cleanup code #2862
Conversation
…se safe access instead of null check, cleanup test comments
| val findings = ReturnCount(TestConfig(mapOf(ReturnCount.EXCLUDE_GUARD_CLAUSES to "true"))) | ||
| .compileAndLint(code) | ||
| assertThat(findings).hasSize(1) | ||
| } | ||
|
|
||
| it("should get flagged for too-complicated guard clause, with EXCLUDE_GUARD_CLAUSES off") { |
There was a problem hiding this comment.
Could you explain why this testcase got removed?
There was a problem hiding this comment.
@arturbosch this test case is redundant, given it is expected to be reported even with the EXCLUDE_GUARD_CLAUSES flag's value being true, it will surely be reported when the flag's value is false.
There was a problem hiding this comment.
Ah now I see. Please rename the testcase to reports a too-complicated if statement for being a guard clause
There was a problem hiding this comment.
Renamed the test case as advised above
detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/GuardClauses.kt
Outdated
Show resolved
Hide resolved
| @@ -30,12 +30,12 @@ fun KtExpression.isGuardClause(): Boolean { | |||
| ?: return false | |||
|
|
|||
| return ifExpr.`else` == null && | |||
| returnExpr === ifExpr.then?.lastBlockStatementOrThis() | |||
| returnExpr == ifExpr.then?.lastBlockStatementOrThis() | |||
There was a problem hiding this comment.
=== is used here on purpose.
The return expr is the same node found as the last statement in the if block.
Or am I missing something?
There was a problem hiding this comment.
I see, so we need to match the reference rather than the structure of the node. @arturbosch if you confirm I will revert this change.
In any case, the test cases pass with the structural equality == check too.
There was a problem hiding this comment.
Yes, please revert this line.
There was a problem hiding this comment.
This is reverted now.
… rename test case as per MR review comment
…r ThrowsCount, add test case for the same in ThrowsCountSpec
|
@arturbosch I have done the suggested changes. I have also found and cleaned up code duplication in ThrowsCount. Please review the same and let me know of any suggestions / clarifications. |
Codecov Report
@@ Coverage Diff @@
## master #2862 +/- ##
=========================================
Coverage 80.65% 80.66%
+ Complexity 2346 2343 -3
=========================================
Files 388 388
Lines 7037 7035 -2
Branches 1288 1288
=========================================
- Hits 5676 5675 -1
Misses 716 716
+ Partials 645 644 -1
Continue to review full report at Codecov.
|
| val returnExpr = this.findDescendantOfType<KtReturnExpression>() ?: return false | ||
| return isIfConditionGuardClause(returnExpr) || returnExpr.isElvisOperatorGuardClause() | ||
| } | ||
|
|
||
| fun KtExpression.isElvisOperatorGuardClause(): Boolean { |
There was a problem hiding this comment.
Could this be internal? I'm a bit worried about the scope polution... I'm going to open an issue to see if we can create some rule for this cases.
There was a problem hiding this comment.
@BraisGabin sounds like a good idea. I would recommend it to be tackled in a separate issue as you suggested.
There was a problem hiding this comment.
Sure, I just created #2863. But, any way, can we make this function internal?
There was a problem hiding this comment.
@BraisGabin I have made this extension function internal (made the other one in the file internal as well)
cc @arturbosch
This PR achieves the following:
?.instead of null checkGuardClauses.ktcommon code forThrowsCountand remove duplicate code.ThrowsCountSpekinternal(inGuardClauses.kt) to avoid scope pollution