Skip to content

ClassOrdering reports a message describing the misorder - #3138

Merged
schalkms merged 5 commits into
detekt:masterfrom
hbmartin:ClassOrdering-descriptive-message
Oct 11, 2020
Merged

ClassOrdering reports a message describing the misorder#3138
schalkms merged 5 commits into
detekt:masterfrom
hbmartin:ClassOrdering-descriptive-message

Conversation

@hbmartin

@hbmartin hbmartin commented Oct 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Currently the ClassOrdering rule reports its own description as a message. This PR modifies the message to report the specific entities identified as out of order.
  • It also slightly refactors the class organization, but requires no test changes, only test changes exist to check message
  • Additional tests (for test coverage sake) describe existing behavior
  • Future PR could improve this by reporting all misordered pairs, not simply the first one: ClassOrdering only reports first misorder, not all misorders #3141
  • Follow up to: New rule: ClassOrdering #3088

@codecov

codecov Bot commented Oct 9, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3138 into master will increase coverage by 0.02%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3138      +/-   ##
============================================
+ Coverage     79.43%   79.46%   +0.02%     
+ Complexity     2597     2591       -6     
============================================
  Files           437      437              
  Lines          7820     7830      +10     
  Branches       1484     1490       +6     
============================================
+ Hits           6212     6222      +10     
  Misses          819      819              
  Partials        789      789
Impacted Files Coverage Δ Complexity Δ
...lab/arturbosch/detekt/rules/style/ClassOrdering.kt 90.00% <87.50%> (+5.00%) 4.00 <1.00> (-6.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 4ce376e...511e8c9. Read the comment docs.

@hbmartin

hbmartin commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

@BraisGabin @schalkms This is my first detekt PR, what is the process for reviews?

BraisGabin
BraisGabin reviewed Oct 10, 2020
View reviewed changes

@BraisGabin BraisGabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great first PR!

Could you add some asserts over the message in the tests? The code that build the message is kind of complex.

And, could you add the missing parts that you found from this rule in a new issue so we can track them? If you want to do those PRs we can assign that issue to you.

@hbmartin

hbmartin commented Oct 10, 2020

Copy link
Copy Markdown
Contributor Author

@BraisGabin thanks!
Added tests for message: 3dc6355
Added an issue for tracking the list of misorders issue: #3141

BraisGabin
BraisGabin reviewed Oct 10, 2020
View reviewed changes
Comment thread detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrderingSpec.kt Outdated
val findings = subject.compileAndLint(code)
assertThat(findings).hasSize(1)
assertThat(findings[0].message).isEqualTo("OutOfOrder (secondary constructor) " +
"should not come before null (class initializer)")

@BraisGabin BraisGabin Oct 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

null is kind of strange here, right?

@hbmartin hbmartin Oct 11, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea, so is Companion (companion) - fixed in 511e8c9

BraisGabin
BraisGabin approved these changes Oct 11, 2020
View reviewed changes

@BraisGabin BraisGabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice job!!

@hbmartin

hbmartin commented Oct 11, 2020

Copy link
Copy Markdown
Contributor Author

@BraisGabin thanks! how does this get merged?

schalkms
schalkms approved these changes Oct 11, 2020
View reviewed changes

@schalkms schalkms left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! @hbmartin 🙂

@schalkms
schalkms merged commit 4722c26 into detekt:master Oct 11, 2020
@hbmartin hbmartin mentioned this pull request Oct 11, 2020
Merged
@arturbosch arturbosch added this to the 1.14.2 milestone Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BraisGabin BraisGabin BraisGabin approved these changes
@schalkms schalkms schalkms approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.14.2

Development

Successfully merging this pull request may close these issues.

4 participants

@hbmartin @BraisGabin @schalkms @arturbosch