Skip to content
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

New rule: ClassOrdering #3088

Merged
merged 4 commits into from
Sep 20, 2020
Merged

New rule: ClassOrdering #3088

merged 4 commits into from
Sep 20, 2020

Conversation

3flex
Copy link
Member

@3flex 3flex commented Sep 18, 2020

A rule to check the contents of a class match this ordering: https://kotlinlang.org/docs/reference/coding-conventions.html#class-layout

I've enabled this for detekt, and you can see in that second commit the changes that were required to comply with the new rule. I think the changes are mostly not controversial, but for those classes where they are a suppression is probably the right way to go, or perhaps changing some extension properties into extension functions.

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #3088 into master will increase coverage by 0.01%.
The diff coverage is 80.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3088      +/-   ##
============================================
+ Coverage     79.28%   79.30%   +0.01%     
- Complexity     2567     2577      +10     
============================================
  Files           434      435       +1     
  Lines          7735     7756      +21     
  Branches       1469     1476       +7     
============================================
+ Hits           6133     6151      +18     
- Misses          816      817       +1     
- Partials        786      788       +2     
Impacted Files Coverage Δ Complexity Δ
...lin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt 66.66% <0.00%> (ø) 0.00 <0.00> (ø)
.../arturbosch/detekt/core/baseline/BaselineFormat.kt 83.33% <0.00%> (ø) 4.00 <0.00> (ø)
.../main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt 29.06% <0.00%> (ø) 18.00 <0.00> (ø)
...ab/arturbosch/detekt/extensions/DetektExtension.kt 47.36% <0.00%> (ø) 6.00 <0.00> (ø)
...o/gitlab/arturbosch/detekt/invoke/DetektInvoker.kt 9.67% <ø> (ø) 0.00 <0.00> (ø)
...in/io/github/detekt/metrics/CognitiveComplexity.kt 89.70% <ø> (ø) 2.00 <0.00> (ø)
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 91.42% <ø> (ø) 5.00 <0.00> (ø)
...tlab/arturbosch/detekt/api/internal/PathFilters.kt 88.00% <33.33%> (ø) 4.00 <3.00> (ø)
...gitlab/arturbosch/detekt/internal/DetektAndroid.kt 70.96% <57.14%> (ø) 12.00 <4.00> (ø)
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Debt.kt 95.83% <83.33%> (ø) 7.00 <4.00> (ø)
... and 16 more

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 793cece...be1c474. Read the comment docs.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great rule 👌 I'm just unsure if we want to enabled it by default as it has the risk of raising a lot of warnings for our users. However the logic is really straightforward and aligned with the Kotlin guidelines.

@3flex
Copy link
Member Author

3flex commented Sep 19, 2020

I'm just unsure if we want to enabled it by default

Sorry I wasn't clear. I've enabled it for detekt scans of the detekt project itself, but it's not enabled by default for users - it's active: false in default-detekt-config.yml. I thought the same thing as you, it could potentially lead to a lot of warnings on larger projects.

I also think that in almost all cases any violations it finds will lead to a more logical class layout so I think there's value in enabling it. It's quick to cut/paste code to fix issues it finds. But for now it's not enabled by default.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this one a lot! Myabe we could add some configuration later but, let's see what the community think when they test it :)

@cortinico
Copy link
Member

Sorry I wasn't clear. I've enabled it for detekt scans of the detekt project itself, but it's not enabled by default for users - it's active: false in default-detekt-config.yml. I thought the same thing as you, it could potentially lead to a lot of warnings on larger projects.

Thanks for the clarification 👌 Looks good to me 🚀

@cortinico cortinico added this to the 1.14.0 milestone Sep 19, 2020
@3flex 3flex merged commit be9bc24 into detekt:master Sep 20, 2020
@3flex 3flex deleted the class-ordering branch September 20, 2020 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants