-
-
Notifications
You must be signed in to change notification settings - Fork 794
Update the implementation of ClassOrdering to handle false negatives #3810
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3810 +/- ##
============================================
- Coverage 83.48% 83.38% -0.11%
+ Complexity 3104 3102 -2
============================================
Files 456 456
Lines 8937 8943 +6
Branches 1751 1755 +4
============================================
- Hits 7461 7457 -4
- Misses 558 568 +10
Partials 918 918
Continue to review full report at Codecov.
|
} | ||
|
||
@JvmInline | ||
@Suppress("MagicNumber", "ModifierOrder") // TODO Remove ModifierOrder once value class is supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pending #3719
Time ago I did some research regarging this rule and the number of issue that it raises. I found that we could use "longest increasing subsequence (LIS)" to reduce the number of issues. But I never implement it. I don't know even if we need to implement it. But as you talk about it I thought it could be usefull. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrdering.kt
Outdated
Show resolved
Hide resolved
this is KtSecondaryConstructor -> Section(1) | ||
this is KtNamedFunction -> Section(2) | ||
this is KtObjectDeclaration && isCompanion() -> Section(3) | ||
else -> null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any alternative to the null
return? Then we could spare the null checks above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kotlin documentation does not specify where we should put nested classes or objects, so the ordering for nested classes shouldn't matter from a static analysis point of view, hence it should not have any priority order - null
is a reasonable choice.
Resolves #3809.
With this new implementation, one noticeable changes are that the number of violations is going to be increased:
For example, in a class layout like
Previous the violation will be reported only the boundary (a <-> b, b <-> c), now it will be reported on (c, d, e, f).
If the increase in the violations is a concern, we can implement grouping logic to reduce the number of violations.