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

Make "ComplexMethod" rule also ignore "return when" if configured #1062

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

dimsuz
Copy link
Contributor

@dimsuz dimsuz commented Aug 21, 2018

If configured with "ignoreSingleWhenExpression", ComplexMethod should
not only skip "when", but also "return when"

Fixes #1054

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.

Looks good to me!

@schalkms
Copy link
Member

schalkms commented Aug 21, 2018

Can you please also test this with single-expression functions. I don't think that the rule reports this atm.

fun f(): Int {
 	return when { ... }
}

@dimsuz
Copy link
Contributor Author

dimsuz commented Aug 21, 2018

Do you mean that this rule should treat if-else-if-else-if-else as a single when? Or the opposite, always report them, irrespective to ignoreSingleWhenExpression flag?

@schalkms
Copy link
Member

@dimsuz Sry I have updated my comment. I meant that the rule should also report a comment if the single expression syntax is used.

@dimsuz
Copy link
Contributor Author

dimsuz commented Aug 21, 2018

Ah, ok! I will test this tomorrow and write back:

  • will set ignoreSingleWhenExpression to false, to force the check
  • run the rule on the file with return when { } containing threshold + 1 number of when-branches
  • the rule should report it as a code smell

@schalkms
Copy link
Member

Okay. I think we were talking at cross purposes. Adding the following snippet as another test case should report a violation. Note that I took your test code and just used the single expression syntax. Atm this case is not handled by the ComplexMethod rule.

fun complexMethodWithSingleWhen4(i: Int) = when (i) {
	1 -> "one"
	2 -> "two"
	3 -> "three"
	else -> ""
}

@dimsuz
Copy link
Contributor Author

dimsuz commented Aug 21, 2018

Oh, right! I got mixed up between "single expression inside the body" and "single expression function syntax" .

Ok, so I should test that your snippet above

  • is causing a violation when ignoreSingleWhenExpression is false (the default)
  • is not causing a violation when ignoreSingleWhenExpression is true

In other words fun test() = when {} should behave exactly as fun test() { return when {} }

I hope I got it right this time 🙂.

I will also add test(s) for these cases.

If configured with "ignoreSingleWhenExpression", ComplexMethod should
not only skip "when", but also "return when"

Fixes detekt#1054
@dimsuz dimsuz force-pushed the improvement-#1054-ComplexMethod branch from 73f25c4 to 5d154eb Compare August 22, 2018 16:53
@dimsuz
Copy link
Contributor Author

dimsuz commented Aug 22, 2018

I have updated the code and the tests to handle a "function single expression" syntax.

In ComplexMethodSpec there is a spec "reports all complex methods" and ignoring is turned off it indeed reports all "single when" cases as a failures.
And when ignoring is turned on only one case without single whens is reported

@dimsuz
Copy link
Contributor Author

dimsuz commented Aug 22, 2018

It turned out that your snippet above is already handled and is reported as ComplexMethod [5/4] when ignoring single when is turned off (though I tested it at home, on current release version of detekt, but it shouldn't have changes I guess). I didn't add a specific test for this case though.

@arturbosch arturbosch merged commit 59bce1c into detekt:master Aug 31, 2018
@arturbosch arturbosch added this to the RC9 milestone Sep 10, 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.

3 participants