Repository navigation
[tool] Migrate InstallCommand to modular dependency injection - #190771
Conversation
8f028b6 to
a7f931f
Compare
a5c116d to
8509286
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors InstallCommand and the installApp function to accept and use ToolContext and an injected Logger instead of relying on ambient globals. The corresponding tests are updated to use a fake tool context and verify logging output. Feedback suggests making the logger parameter required in installApp to prevent warnings and errors from being silently dropped.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors InstallCommand and the installApp function to inject ToolContext and Logger rather than relying on global variables, and updates the unit tests to use fakes. The review feedback suggests simplifying the InstallCommand constructor by using Dart's super-initializer parameters, which would eliminate the need for the redundant private _toolContext field and its getter.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors InstallCommand and the installApp function to use ToolContext and an injected Logger instead of relying on global variables. It also updates the corresponding tests to use fake contexts and loggers, removing context overrides. Feedback was provided regarding a test description that violates the Flutter style guide by starting with an uppercase letter and ending with a period.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors InstallCommand and the installApp function to use ToolContext and Logger instead of relying on global variables, and updates the corresponding tests to use a fake tool context and buffer logger. The reviewer suggests introducing a private getter for the logger in InstallCommand to reduce code duplication across its methods.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors InstallCommand and the installApp function to depend on ToolContext and Logger instead of global variables, updating the test suite to use explicit fakes rather than context overrides. The review feedback recommends explicitly typing the toolContext parameter in the InstallCommand constructor as ToolContext to prevent compile-time nullability inference issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors "InstallCommand" and the "installApp" function to remove dependencies on global variables by explicitly passing and utilizing "ToolContext" and "Logger". The corresponding unit tests in "install_test.dart" have been updated to use "FakeToolContext" and "BufferLogger" instead of context overrides, and new tests have been added to verify logging behavior. I have no feedback to provide.
Summary
Part 13 of the modular dependency injection migration.
Migrates
InstallCommandto modular dependency injection containerToolContext, eliminating ambient global references and adding hermetic unit tests.Part of #47161