Repository navigation
[tool] Migrate BuildAarCommand to modular dependency injection - #192247
Conversation
…endency injection
…via executable.dart
…ldCommand and AndroidGradleBuilder
…oidGradleBuilder in executable.dart
Extract Logger, Terminal, FileSystem, and Platform from ToolContext using object pattern destructuring for diff reduction in build_apk, build_appbundle, and build_aar.
…in Android build commands Ensure AndroidBuilder is required and non-nullable in BuildAarCommand, BuildApkCommand, and BuildAppBundleCommand. Construct effective AndroidGradleBuilder in BuildCommand if not injected. Update tests to remove context.get<AndroidBuilder>() and context.get<Analytics>().
…ate fallback globals Require non-nullable `AndroidBuilder` parameter in `BuildCommand` to eliminate fallback `AndroidGradleBuilder` construction within the command body. Require `Analytics` and `Logger` in `exitWithNoSdkMessage` to remove ambient `globals` fallback references. Simplify test runners across permeable Android build tests.
…separate PRs Narrow scope of PR 14 to `AndroidGradleBuilder` constructor DI, `exitWithNoSdkMessage` refactoring, non-nullable `AndroidBuilder` in `BuildCommand`, and `BuildApkCommand` migration.
Migrate `BuildAppBundleCommand` (`flutter build appbundle`) to modular dependency injection with `ToolContext`, `AndroidContext`, `BuildSystem`, and `AndroidBuilder`, removing direct dependencies on `globals.dart`.
Migrate `BuildAarCommand` (`flutter build aar`) to modular dependency injection with `ToolContext`, `AndroidContext`, `BuildSystem`, and `AndroidBuilder`, removing direct dependencies on `globals.dart`.
bkonyi
added
the
tool
github-actions
Bot
added
a: desktop
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors BuildAarCommand to accept its dependencies (AndroidBuilder, AndroidContext, BuildSystem, and ToolContext) directly through its constructor rather than relying on global context or individual fields. Corresponding updates were made to BuildCommand and the associated tests to pass these new parameters. Review feedback recommends adding documentation to the newly introduced public getters in BuildAarCommand to adhere to the style guide, and removing a redundant local logger variable that shadows an inherited member.
| final Logger logger = toolContext.logger; | ||
| if (androidSdk == null) { | ||
| exitWithNoSdkMessage(analytics: analytics, logger: logger); | ||
| } |
There was a problem hiding this comment.
The local variable logger is redundant because logger is already inherited from FlutterCommand (via BuildSubCommand). Removing it simplifies the code and avoids shadowing the inherited member.
if (androidSdk == null) {
exitWithNoSdkMessage(analytics: analytics, logger: logger);
}References
- Suggest simplification and refactoring: Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| '''); | ||
|
|
||
| final BuildCommand command = createFakeBuildCommand( | ||
| androidSdk: FakeAndroidSdk(), |
| final Directory dotAndroidDir = fs.directory('.android')..createSync(recursive: true); | ||
| dotAndroidDir.childFile('gradlew').createSync(); | ||
|
|
||
| processManager.addCommands(<FakeCommand>[ |
There was a problem hiding this comment.
why dont we need this anymore?
There was a problem hiding this comment.
At this line we were still accessing context.get<AndroidBuilder>() (androidBuilder is a getter defined on BuildCommand) instead of the FakeAndroidBuilder passed to BuildAarCommand. Since we're not longer using a "real" instance, we don't have these process invocations anymore.
There was a problem hiding this comment.
Ah and using a real instance or not doesn't matter for this test. gotcha.
flutter-dashboard
Bot
removed
the
autosubmit
2 participants
Summary
Part 14c of the modular dependency injection migration.
Migrates
BuildAarCommand(flutter build aar) to modular dependency injection withToolContextandAndroidContext, removing direct dependencies onglobals.dart.Stacked on #192246
Compare: bkonyi/flutter@master...di/14c-build-aar
Part of #47161