detekt: Issues raised even when "on" threshold

Hi everyone! We incorporated detekt in our project a while ago and, while we like it very much, we’ve been noticing that it seems to report issues even when the checks are “on” the threshold and not after crossing it.

Expected Behavior

For this example, imagine any function, with three nested blocks.

fun myFunction(): Boolean {
  someNullableVariable?.let {
    if (something) {
      // do something
      if (something else) {
        return true
      }
  }
  return false
}

We expect detekt not to report anything, since it’s configure like so:

  NestedBlockDepth:
    active: true
    threshold: 3

Observed Behavior

The function is reported to have infringed the NestedBlockDepth rule:

NestedBlockDepth - 3/3 - [applyInternal] at <path>/<file>.kt:16:18

My team and I believe that this should not be reported, since the number of nested blocks is 3 from the allowed 3 nested blocks. The issue should only be raised, when the set threshold is crossed, for instance at an event where 4/3 could be observed.

Steps to Reproduce

  • Write a similar function
  • Have detekt in your project with the same settings for NestedBlockDepth

Context

This does not only happen with NestedBlockDepth, but every single rule is treated as such in detekt. While we want to use detekt to ensure code quality, we do not want to have “add one to the real threshold” or suppress the rule in order to get away with it. Also after a while the question could become “Was this a 3 or a 4? The config says 4, so why is 4 not allowed?”, mainly if new colleagues come to the project, or the ones that instituted detekt and the rules move to another project.

Your Environment

  • Version of detekt used: 1.15.0
  • Version of Gradle used (if applicable): 6.8
  • Operating System and version: macOS Big Sur 11.2.3

About this issue

  • Original URL
  • State: open
  • Created 3 years ago
  • Reactions: 3
  • Comments: 15 (10 by maintainers)

Commits related to this issue

Most upvoted comments

I’ll vote to change it. And I think that it should be done in 2.0.

+3
BraisGabin on Nov 6, 2021

There were 4 votes for the current solution and 1 for the one you suggested. Furthermore, all maintainers at this time voted for the currently implemented solution. As a side note, tools like pmd use the same semantic for thresholds as detekt does. It’s a decision that needs to be taken at the beginning for obvious reasons.

Thanks for the clarification @schalkms, I could only read the discussion and couldn’t follow the decision process. You’re right, it’s an important decision that must be made in the very beginning.

What do you miss here? @jotabe87 https://detekt.github.io/detekt/configurations.html#rule-sets-and-rules

No, I didn’t. But I believe it’s a problem of semantics. The excerpt reads: “For example if you want to allow up to 20 functions inside a Kotlin file instead of the default threshold of 10, write:” and gives the following configuration code

complexity:
  TooManyFunctions:
    threshold: 20

The above sentence reads up to 20, which means “less than or equal to, but not more than, a stated value, number, level, or time” according do the dictionary. In actuality, if I want to allow 20 functions inside a Kotlin file I have to set the threshold to 21. If I set it to 20 as the documentation suggests, I will be allowing for 19 functions to be written in the file. Likewise, detekt doesn’t allow 10 functions in a file by default, but rather 9. Do you see the confusion here?

Let me start to say thanks on behalf of the detekt team for your valuable to time to give feedback in this repo. Your feedback is really appreciated.

Thanks, we really like the tool and it’s already been very helpful!

I never understood the current behaviour either, because I think of a threshold as a limit. But you could also say “it meets the threshold” for reporting. The naming is ambiguous so people have different opinions.

I agree with @3flex, the biggest problem here is the ambiguity. So if you decide to change or not to change the way detekt works, either way is fine, but please change the documentation to make it clearer.

I’m in favour of changing the status quo. We could do this by introducing new config option names, aliasing and deprecating “threshold” and ThresholdRule, so we have a nice migration path. We wouldn’t have to wait for version 2.0 to do this.

Imho I believe it should be changed, even if it’s a breaking change. It should ofc be well communicated, with deprecation warnings and “whatnots”, so that no one loses trust on the tool when it changes. But that’s just my 2¢ as a user.

+2
jotabe87 on Apr 21, 2021

I never understood the current behaviour either, because I think of a threshold as a limit. But you could also say “it meets the threshold” for reporting. The naming is ambiguous so people have different opinions.

pmd is clearer with the name of the setting. On a nested-if rule the threshold is named “problemDepth” which is less ambiguous to me since it reads to me like “this is the depth at which there’s a problem” so I expect a report when that depth is reached.

I’m in favour of changing the status quo. We could do this by introducing new config option names, aliasing and deprecating “threshold” and ThresholdRule, so we have a nice migration path. We wouldn’t have to wait for version 2.0 to do this.

+1
3flex on Apr 18, 2021
Read more comments on GitHub
← detekt: Getting "io.gitlab.arturbosch.detekt.api.RuleSetProvider: Error reading configuration file" error sometimes
detekt: K2 compiler crashes Detekt →