-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Clean dependency up at the modules #2044
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Jaehwa-Noh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Gradle dependency declarations across several modules to adhere more closely to recommended practices regarding Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
api dependency up at the core:ui moduelapi dependency up at the core:ui module
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.
Code Review
This pull request aims to refine dependency configurations in the :core:ui module by replacing api with implementation. While this is a good practice for encapsulation, the change for the androidx.metrics dependency is incorrect. The :core:ui module exposes types from androidx.metrics in its public API, which requires the use of the api configuration. Consequently, the addition of androidx.metrics as a direct dependency in the :app module is a workaround for this incorrect change. I've provided detailed comments on these points.
|
/gemini review |
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.
Code Review
This pull request aims to improve dependency encapsulation in the :core:ui module by changing api dependencies to implementation. This is a good practice that can lead to faster builds and a cleaner dependency graph. The related changes to add explicit dependencies in consumer modules like :feature:foryou and :feature:search are also correct.
However, I've found one case where a dependency was incorrectly changed to implementation. The androidx.metrics library is still exposed through the public API of :core:ui, and therefore should remain an api dependency. I've provided a detailed comment and a suggestion to fix this.
8d5c1b2 to
e6e7d1a
Compare
|
/gemini review |
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.
Code Review
This pull request aims to clean up Gradle dependencies by replacing api with implementation where appropriate in the :core:ui module. This is a great initiative for improving build performance and encapsulation.
My review identifies a critical issue with how the :core:analytics dependency is handled. The :core:ui module exposes types from :core:analytics in its public API, which requires the dependency to be declared with api, not implementation. The current change incorrectly sets it to implementation and works around the resulting build errors by adding explicit dependencies in consumer modules.
My main suggestion is to revert the :core:analytics dependency in :core:ui back to api. This is the correct way to model this relationship and will allow you to remove the new explicit dependencies from the feature modules. I've also suggested changing the androidx.metrics dependency to implementation as it appears to be an internal detail.
If these suggestions are applied, please also remember to update the dependency graphs in the various README.md files to reflect the correct dependency structure.
Change-Id: I3ce3b4033796cbfdf05e5a8990c96fa1905c87a5
Change-Id: I95815c1551b6aa4653f333084608dff2f039ec7e
e6e7d1a to
6ab7cc1
Compare
api dependency up at the core:ui module|
/gemini review |
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.
Code Review
This pull request correctly refactors Gradle dependencies to improve module encapsulation by replacing api with implementation where dependencies are not meant to be transitively exposed. Specifically, core:data no longer declares a direct dependency on core:common (as it's provided transitively), and core:ui now uses implementation for core:designsystem. All corresponding module dependency graphs in the README.md files have been updated to reflect these changes. I've added one suggestion to sort dependencies in core/ui/build.gradle.kts for better maintainability. Overall, this is a good cleanup.
Change-Id: I7923dcd7fdb2ac4777359545b1ca88262f1d0578
What I have done and why
According this documentation
https://docs.gradle.org/current/userguide/declaring_configurations.html#android_gradle_plugin
apishould use when exports that class dependency to the consumer in current module.I replaced
apitoimplementationfor proper propaganda about up to date state and remove unused dependency.