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

Added rule UnnecessaryInnerClass #4394

Merged
merged 8 commits into from
Jan 23, 2022

Conversation

severn-everett
Copy link
Contributor

This addresses issue #875.

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #4394 (1ce0a2d) into main (21a751a) will increase coverage by 0.05%.
The diff coverage is 92.10%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4394      +/-   ##
============================================
+ Coverage     84.34%   84.39%   +0.05%     
- Complexity     3299     3324      +25     
============================================
  Files           473      474       +1     
  Lines         10532    10605      +73     
  Branches       1885     1899      +14     
============================================
+ Hits           8883     8950      +67     
- Misses          671      672       +1     
- Partials        978      983       +5     
Impacted Files Coverage Δ
...rbosch/detekt/rules/style/UnnecessaryInnerClass.kt 91.89% <91.89%> (ø)
...in/io/github/detekt/metrics/CognitiveComplexity.kt 88.23% <100.00%> (-0.28%) ⬇️
...rturbosch/detekt/rules/style/StyleGuideProvider.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 21a751a...1ce0a2d. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this very old issue! I think this rule fits nicely into detekt.
As mentioned in the issue description, there's a high risk for false positives though. Hence, this rule should be tested thoroughly on sample projects. Have you run this rule on other codebases? If not, the following script in detekt might help to achieve that.

https://github.com/detekt/detekt/blob/main/scripts/get_analysis_projects.kts

}
""".trimIndent()

Assertions.assertThat(subject.lintWithContext(env, code)).hasSize(1)
Copy link
Member

Choose a reason for hiding this comment

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

The Assertions. reference can be imported and thus removed from the assert statements for a cleaner test structure.
Other rules show examples on how to use it.

@severn-everett
Copy link
Contributor Author

severn-everett commented Dec 27, 2021

Thanks for tackling this very old issue! I think this rule fits nicely into detekt. As mentioned in the issue description, there's a high risk for false positives though. Hence, this rule should be tested thoroughly on sample projects. Have you run this rule on other codebases? If not, the following script in detekt might help to achieve that.

https://github.com/detekt/detekt/blob/main/scripts/get_analysis_projects.kts

The rule requires the binding context to be present, which would mean configuring the classpath option when running Detekt on each of the sample projects. Is there a way to determine the classpaths for these projects programatically? Otherwise, as they are both non-trivial and externally-managed, a manually-maintained list of classpaths for each project would be rather fragile. Given that this is not going to be an active rule by default - and thus won't be breaking workflows automatically when users upgrade Detekt - I would say to publish the rule in the next release and address the issues as they come.

@schalkms
Copy link
Member

I'm still hesitant to merge this, since I feel like the rule could take a few more test runs. Testing it in production is not the route, which I'd like to go here.

@severn-everett
Copy link
Contributor Author

Why not? Releasing experimental and beta code is not an unusual workflow, e.g. the pattern matching preview code that's been released in JDK 16 and 17; and given that the rule will be disabled by default, no user's going to have their code broken by any issues with this rule unless they actively go out of their way to enable it. It'll be a lot more straightforward to be able to test the new rule when it can be integrated via the Gradle plug-in; otherwise, it's going to be a very arduous task to hand-build the classpath for all of these projects to test out the rule manually.

@chao2zhang
Copy link
Member

Alternatively, we could enable this new rule for the detekt project itself to help us evaluate. I agree with @severn-everett that since this is a new rule disabled by default, we can roll it out to production and gather feedback.

A counterpart is UnusedPrivateMember, this is a rule that seems to require constant maintenance. I don't think UnnecessaryInnerClass would require as much load as UnusedPrivateMember.

}
}

// Replace this "contructor().apply{}" pattern with buildSet() when Kotlin is upgraded to 1.6
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace it now? #4277

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll swing back and fix this tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like it's available until the API version is at 1.6; I'll update the note to reflect that

@cortinico cortinico added this to the 1.20.0 milestone Jan 5, 2022
@cortinico cortinico added notable changes Marker for notable changes in the changelog rules labels Jan 5, 2022
@chao2zhang chao2zhang merged commit 31520da into detekt:main Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants