UnderscoresInNumericLiterals acceptableDecimalLength is off by one

[BraisGabin]

This rule was using the word "decimal" everywhere. It's correct to call a base 10 number "decimal" but it is saying the same thing twice. And the problem with "decimal" is that it have a lot of meanings so it can be missleading. For that reason I changed the documentation a bit removing the word "decimal" (first commit).

After that the only point where "decimal" was used is in the configuration so to avoid the breaking change and because it also improves "readability" I renamed "acceptableDecimalLength" to "acceptableLenght". All this rule is about base 10 numbers so there's is no need to point that out in the configuration too.

But this is a Draft an it's not a ready to merge PR because I found a missing feature in our new configuration system:

private val acceptableLength: Int by configWithFallback("acceptableDecimalLength", 4)

We can't use the value of acceptableDecimalLength as the value of acceptableLenght because it would be off by one. Should we care about this? We were talking about adding a "breaking change" to fix this so, maybe, we should not care.

Fix #3860

[marschwar]

I think we should care and not introduce a breaking change like that.

I agree that the fallback config property is missing some functionality. I was looking into it and ended up with something like

    @Configuration("Length under which decimal base 10 literals are not required to have underscores")
    @Deprecated("Use `acceptableLength` instead. Make sure to read the documentation as the logic has changed.")
    private val acceptableDecimalLength: Int by config(5)

    @Configuration("Maximum number of digits that a number can have and not use underscores")
    private val acceptableLength: Int by configWithFallback(::acceptableDecimalLength, 4)

I know that @chao2zhang was asking about something like this from the beginning but at the time I could not get this to work.

In the implementation of the config property I get the message: Call uses reflection API which is not found in compilation classpath. Make sure you have kotlin-reflect.jar in the classpath I am unsure if it is safe to ignore this warning. I will create a PR to illustrate the solution.

[BraisGabin]

That api doesn't allow me to convert from one value to the other, I mean, do the - 1

[marschwar]

🤔 You could do val acceptableDecimalLength: Int by config(5) { it - 1 }. So whenever the fallback property is used, the value is reduced by one. (I have not tested that yet).

[BraisGabin]

🤔 You could do val acceptableDecimalLength: Int by config(5) { it - 1 }. So whenever the fallback property is used, the value is reduced by one. (I have not tested that yet).

Good point! That have sense.

[marschwar]

With the current implementation of the configWithFallback this will not work. This relies on #3982 in order to work.

[BraisGabin]

Rebased and now it uses the new version of configWithFallback.

[codecov]

Codecov Report

Merging #3972 (e7c5c47) into main (7962979) will increase coverage by 0.00%.
The diff coverage is 83.33%.

❗ Current head e7c5c47 differs from pull request most recent head 27117e6. Consider uploading reports for the commit 27117e6 to get more accurate results

@@            Coverage Diff            @@
##               main    #3972   +/-   ##
=========================================
  Coverage     83.48%   83.48%           
  Complexity     3195     3195           
=========================================
  Files           461      461           
  Lines          9112     9113    +1     
  Branches       1774     1774           
=========================================
+ Hits           7607     7608    +1     
  Misses          571      571           
  Partials        934      934           

Impacted Files	Coverage Δ
...detekt/rules/style/UnderscoresInNumericLiterals.kt	81.57% <83.33%> (+0.49%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7962979...27117e6. Read the comment docs.

[marschwar]

Please regenerate the default config to use the the property name.

Should we also update the test to use the new config property and keep only one or two with the deprecated value?

[BraisGabin]

Please regenerate the default config to use the the property name.

And not only the default config. The deprecation list too! We merged the PR just in time 😂.

Should we also update the test to use the new config property and keep only one or two with the deprecated value?

You are right! Done :)