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 unnecessary apply rule #1229

Merged
merged 3 commits into from
Oct 20, 2018
Merged

Conversation

arjank
Copy link
Contributor

@arjank arjank commented Oct 10, 2018

This PR adds a rule to check for unnecessary use of apply, as described in issue #1214.

}""")
assertThat(findings).hasSize(0)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about a test case where apply is called with unnecessary parentheses a.apply({ test = true }).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be covered by the existing UnnecessaryParentheses check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true. But not only that, it would also be an unnecessary apply. If we add this test we ensure that the user doesn't run detekt, realizes the UnnecessaryParentheses, fixes it, runs detekt again only to see yet another issue on the exact line of code.
If we ensure both rules report at the same time it gives the report more information that the user can act on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've added some more tests, including one to cover this scenario.

@Mauin
Copy link
Collaborator

Mauin commented Oct 11, 2018

Thank you for the contribution!

@egor-n
Copy link
Contributor

egor-n commented Oct 12, 2018

Would also be nice to add a test case where the return value of apply is used:

fun a(i: Int) {
}

a(1.apply {
  println(this)
})

Adds a rule, some tests for it, and also updates the StyleGuideProvider
to include this new rule.
@arjank
Copy link
Contributor Author

arjank commented Oct 12, 2018

Would also be nice to add a test case where the return value of apply is used:

fun a(i: Int) {
}

a(1.apply {
  println(this)
})

I've added some tests for this.

Copy link
Member

@schalkms schalkms 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 implementing this. I like this rule.

val findings = subject.lint("""
fun b(i : Int) {
}
fun f() {
Copy link
Member

Choose a reason for hiding this comment

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

Ca we please use separate test cases for each code sample. This makes it easier to debug and trace it.

val findings = subject.lint("""
fun b(i : Int) {
}
fun f() {
Copy link
Member

Choose a reason for hiding this comment

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

The same applies here.

Copy link
Collaborator

@Mauin Mauin left a comment

Choose a reason for hiding this comment

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

Thank you for the additional tests! As @schalkms mentioned it makes sense splitting them into individual test cases to make them easier to maintain and more self-explanatory.

Copy link
Member

@arturbosch arturbosch 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 the contribution!

@arturbosch arturbosch merged commit ea1f94a into detekt:master Oct 20, 2018
@arturbosch arturbosch added this to the RC10 milestone Nov 2, 2018
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