Add an alias for FunctionMinLength/FunctionMaxLength rules to be more descriptive

[mohamed-elmahdi]

Renaming the following rules (FunctionMinLength, FunctionMaxLength) to (FunctionNameMaxLength, FunctionNameMinLength) as these rules validate the length of the function names not the functions themselves making them more descriptive to either a user reading the code or a user reading a report.

[cortinico]

This is a breaking change and I don't believe it will land as it is.

I suggest we eventually add an alias for those rules like:

    override val defaultRuleIdAliases: Set<String>
        get() = setOf("FunctionNameMaxLength")

[mohamed-elmahdi]

This is a breaking change and I don't believe it will land as it is.

I suggest we eventually add an alias for those rules like:

    override val defaultRuleIdAliases: Set<String>
        get() = setOf("FunctionNameMaxLength")

Added descriptive aliases for these rules. One thing concerns me though, will the new aliases appear on the reporting of the issues? or will this just change the interface through which people suppress these rules?

[cortinico]

or will this just change the interface through which people suppress these rules?

^ This
Plus it should work also for the config file.

[codecov]

Codecov Report

Merging #4050 (0aef2a9) into main (3ecac0d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #4050   +/-   ##
=========================================
  Coverage     83.58%   83.58%           
- Complexity     3187     3189    +2     
=========================================
  Files           459      459           
  Lines          9101     9103    +2     
  Branches       1772     1772           
=========================================
+ Hits           7607     7609    +2     
  Misses          561      561           
  Partials        933      933           

Impacted Files	Coverage Δ
...rturbosch/detekt/rules/naming/FunctionMaxLength.kt	100.00% <100.00%> (ø)
...rturbosch/detekt/rules/naming/FunctionMinLength.kt	100.00% <100.00%> (ø)

---

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 3ecac0d...0aef2a9. Read the comment docs.

[mohamed-elmahdi]

Does it usually take this long to get reviewed? Or is there something missing form my side delaying the process?

[cortinico]

Does it usually take this long to get reviewed? Or is there something missing form my side delaying the process?

Sorry I was waiting for other maintainer feedbacks. Thanks for bumping.
The code looks good on my end. I'm more wondering on if we should merge this or not mostly for two reasons:

1. What you said: or will this just change the interface through which people suppress these rules. If users experienced confusion because of the name of the rule in the report, this PR won't help.

2. I'm unsure if there is a wider agreement on making this change. While the alias you're suggesting are definitely clearer than the original rule name, I wanted to make sure other maintainers also agree on this (we want to avoid that everyone stars adding alias to rules with their "preferred name" as it will get harder to maintain).

EDIT: grammar

[mohamed-elmahdi]

Valid points. I'll investigate later if I can change the rule name (class name) itself without breaking anything, this will unify the naming and eliminate the confusion problem. Would you be kind enough to give me a hint of what this change would break? the last time I tried to make this change, The failing tests were the ones checking for the rule names (Class names). I figured that these were made for some reason but I couldn't fish it out.

[cortinico]

I'll investigate later if I can change the rule name (class name) itself without breaking anything, this will unify the naming and eliminate the confusion problem.

Please note that you can also "copy" those rules to your own custom rules. This will allow you to rename them as you wish and to don't have an alias at all (it will create less confusion for your users).

if I can change the rule name (class name) itself without breaking anything,
Would you be kind enough to give me a hint of what this change would break?

Renaming a rule inside Detekt is always a breaking change and we never do that (unless we ship a major version). The reason is that this is going to affect all the users that were using that rule and, unless something really critical happened, it's not a cost we're willing to pay.

[BraisGabin]

Sorry for the late feedback. I thought that this rule was still about renaming the rules, and I saw already cortinico saying no so there was not point to say the same.

I don't see any problem with the current implementation. This will just improve the suppression experience. You could use @Supress("FunctionMaxNameLength") or @Supress("FunctionMaxLength") and both will work. But nothing else will change. Is that what we want to archive here? If so, I agree with merging this.

I do agree that the name of these two rules is bad and we should change it but I think that the way to fix this is to create a new issue and mark it as a breaking change and do it for detekt 2.0