-
-
Notifications
You must be signed in to change notification settings - Fork 757
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
ClassOrdering rule reports a list of errors #3142
Conversation
hbmartin
commented
Oct 11, 2020
- Addresses ClassOrdering only reports first misorder, not all misorders #3141 where only the first misorder in a class is reported
- Also report entity where error occurred, not just parent class
- Both of these changes might break existing baselines? - not sure how to address this
- Follow up to ClassOrdering reports a message describing the misorder #3138
Also report entity where error occurred not just parent class
Codecov Report
@@ Coverage Diff @@
## master #3142 +/- ##
============================================
+ Coverage 79.46% 79.52% +0.05%
- Complexity 2591 2592 +1
============================================
Files 437 437
Lines 7830 7852 +22
Branches 1490 1495 +5
============================================
+ Hits 6222 6244 +22
+ Misses 819 817 -2
- Partials 789 791 +2
Continue to review full report at Codecov.
|
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 addressing this issue and further improving this rule!
Please see my attached comments in regards to two minor improvements.
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrdering.kt
Outdated
Show resolved
Hide resolved
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrdering.kt
Show resolved
Hide resolved
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.
Good one!
I'm not sure if this is the best way to report all the errors. I mean, we can be reporting more than the actual movements that you need to do. This is the same problem as find the minimum insertions to sort an array (related links) I'm not against merging this because it's better than the current implementation but I think that we should keep an issue to track this and implement it to report the proper number of issues. |
@BraisGabin made an issue to track your comment: #3144 , please assign to me 😃 |
And based on that find the minimal violation Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived
And based on that find the minimal violation Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived
And based on that find the minimal violation Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived
And based on that find the minimal violation Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived
And based on that find the minimal violation Original implementation comment at detekt#3142 (comment) which suggested the [algo](https://www.geeksforgeeks.org/minimum-insertions-sort-array/) from which this change is derived