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: Unused kotlin extensions #440

Merged
merged 10 commits into from
Mar 14, 2022

Conversation

tasomaniac
Copy link
Contributor

Kotlin Android Extensions are officially deprecated and will be completely removed as part of Kotlin 1.8

This new rule checks if it is unused and can be removed from a module

@tasomaniac tasomaniac requested a review from RBusarow as a code owner March 9, 2022 21:13
@tasomaniac tasomaniac changed the title New rules: Unused kotlin extensions New rule: Unused kotlin extensions Mar 9, 2022
import modulecheck.parsing.gradle.Declaration
import modulecheck.project.McProject
import modulecheck.utils.LazyDeferred
import modulecheck.utils.lazyDeferred
import java.io.File

data class UnusedKaptPluginFinding(
data class UnusedPluginFinding(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this generic. Can be used with any unused Gradle plugin

Copy link
Member

Choose a reason for hiding this comment

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

This is good!

The kapt-oriented stuff is just about the oldest code in the project, and largely unchanged. It doesn't quite fit the patterns of everything else, but it hasn't been bad enough for me to revisit it just yet.

At some point, I'm going to want to broaden the somewhat-dated KAPT checks to a more generic "code generation" scope which includes Anvil and KSP extensions.

So, this is a step in that direction. :)

@tasomaniac tasomaniac force-pushed the unused-kotlin-extensions-rule branch from e4576d2 to d2b7985 Compare March 9, 2022 21:20
@tasomaniac tasomaniac force-pushed the unused-kotlin-extensions-rule branch from d2b7985 to 2059703 Compare March 9, 2022 21:20
@tasomaniac
Copy link
Contributor Author

The versioning CI workflow failed but I'm not sure if that is related to the PR.

Copy link
Member

@RBusarow RBusarow 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 this!

Don't forget that the legacy plugin also handles @Parcelize, which fortunately has a different namespace than the current version.

I added a check for that reference to the rule, and added a test to ensure that rule doesn't give a false positive for "unused" when that older annotation is referenced.

@RBusarow RBusarow added automerge New Rule Something new to check for labels Mar 14, 2022
@kodiakhq kodiakhq bot merged commit 6e091d8 into rickbusarow:main Mar 14, 2022
@tasomaniac
Copy link
Contributor Author

Thanks. It is great that GitHub now allows you to push into forked repos when PR is open. This was a challenge in the past.

The Parcelize is there but isn't that now a separate plugin? If synthetic imports are not used, this plugin should be removed. It should be replaced by Parcelize specific one

@RBusarow
Copy link
Member

The two plugins use different annotation classes for @Parcelize.

old: kotlinx.android.parcel.Parcelize
new: kotlinx.parcelize.Parcelize

So it could technically still be auto-fixed by replacing the plugin, but it would also require updating the Kotlin source.

@tasomaniac tasomaniac deleted the unused-kotlin-extensions-rule branch March 15, 2022 15:52
@tasomaniac
Copy link
Contributor Author

I see. I did not know about the package name change.

@RBusarow RBusarow added the feature New feature or request label Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge feature New feature or request New Rule Something new to check for
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants