-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Prep work for JVM alternatives to modules/plugins #923
Conversation
730f019
to
21078a0
Compare
build-logic/convention/src/main/kotlin/JvmHiltConventionPlugin.kt
Outdated
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.
LGTM, might need some rework when the KSP migration in #933 merged. Will postpone merging this till that's in.
The notification icons are now stored in `:core:notifications`. This forces `:sync:work` to depend on it. Another solution could be to provide the resource id through Hilt, but it would require more changes.
b5a1986
to
61c7d71
Compare
@tunjid now that Hilt KSP has been merged, I've update this review to take advantage of it. |
build-logic/convention/src/main/kotlin/AndroidHiltConventionPlugin.kt
Outdated
Show resolved
Hide resolved
build-logic/convention/src/main/kotlin/JvmHiltConventionPlugin.kt
Outdated
Show resolved
Hide resolved
I added my review just in case we decide to keep the "2 concrete classes -> 1 abstract class" approach, however, I'm leaning towards your suggestion of an if/else check and a single |
Let's try the if/else version then :) |
You didn't even need an if/else :) |
Some modules, like
:core:datastore
probably don't need to be "Android" aware.And this PR prepares the work to transform them into simpler JVM modules.
I've decided to go with an abstract
HiltConventionPlugin
and implements two concrete classes:AndroidHiltConventionPlugin
JvmHiltConventionPlugin
But this could also be solved with a if/else check based on
pluginManager.hasPlugin("com.android.base")
, tell me if you prefer this solution.