Implement rule UnnecessaryPartOfBinaryExpression
#5203
Conversation
cortinico
changed the title
UnnecessaryPartOfBinaryExpression
...in/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt
Outdated
Show resolved
Hide resolved
| super.visitBinaryExpression(expression) | ||
| val children = expression.children.map { it.text.replace(Regex("\\s"), "") } | ||
|
|
||
| if (children.size != children.distinct().size) { |
There was a problem hiding this comment.
Are we missing any potential false positives here?
There was a problem hiding this comment.
No. I can see possibility for true negative case. In case when condition has extra brackets. Like a (foo && (foo)). I did not add a test for the case because detekt has a forbidden brackets rule.
We can change the condition, and compare contents or add some preprocessing to the children remove the && and ||.
Do we need this improve ?
There was a problem hiding this comment.
Do we need this improve ?
Not necessarily. The rule "looked" too easy and I wanted to collect some feedback around it. Let's wait for other maintainers' opinions but it should be good to go 👍
…detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
|
Don't get me wrong, but is this really a problem in die wild that needs its own rule? I know this is not a binary expression but this is essentially the same thing and not detected by the rule. fun bar() {
val foo = true
val baz = false
if (foo || baz || foo) {
//TODO
}
}I know that it does not make much sense but an |
|
@marschwar The rule will report an issue in the line |
I tried and it does not. |
|
Ok, I added the test case, and fix them. |
|
@marschwar I added your test case and several new tests and rewrite the rule. |
I have same the impression... Does this rule "borns" from a real problem in your code base? Could you explain your use case for this rule? |
Yes, I started a rule development because I detect something like |
|
@BraisGabin are you fine merging this? |
Thanks!
Yes, I'm :) |
|
The check is red after merge the pull request. It happened because the merge request pipeline executed before new rules applied to the repository. Do I need to create a new MR for fix the issues ? Is the currenty pipline for pull request correct ? (Looks like the repo need one more check [is branch are up to date with main]) |
|
Yes, please, could you create a PR solving that? And about a new check... Yes, we could have it but I think that we should rethink our pipelines in general, we have far too much. They are overwhelming to me so they could be even scary for a new contributor. |
|
will be done today |
This can be enforced at the Github level (inside the branch protection rule). It would create a lot of friction though as it will require every PR to be rebased before merging. I'm fine with some failures like this one from time to time as the number of PR we receive is not that high to require extra tooling (like merge queues or other checks). |
|
@BraisGabin fixed #5280 |
Implement the UnnecessaryPartOfBinaryExpression. The rule works with all binary expression include if and when condition. The rule also works with all predicates.