Break pub tools out into their own support mixin #82
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
kenzieschmoll
added
the
dart_tooling_mcp_server: pub tools
| import '../utils/process_manager.dart'; | ||
|
|
||
| // TODO: migrate the analyze files tool to use this mixin and run the | ||
| // `dart analyze` command instead of the analyzer package. |
There was a problem hiding this comment.
This is old and can be removed
| // TODO: migrate the analyze files tool to use this mixin and run the | ||
| // `dart analyze` command instead of the analyzer package. | ||
|
|
||
| /// Mix this in to any MCPServer to add support for running Dart CLI commands |
| implements ProcessManagerSupport { | ||
| @override | ||
| FutureOr<InitializeResult> initialize(InitializeRequest request) { | ||
| if (request.capabilities.roots == null) { |
There was a problem hiding this comment.
We don't necessarily have to do this now, but I do think we should trend more in the direction of just logging a warning, and then not registering the pub tools. See the analyzer support mixin for an example.
Note that warnings have to be logged after initialization completes.
Basically the goal is to still allow the functionality that does work instead of failing initialization entirely.
There was a problem hiding this comment.
Actually, on second look this does not require roots anyways (unless we wanted to make "roots" be optional, and by default run commands in all known roots, but this doesn't actually list the roots today).
|
|
||
| final paths = (rootConfig['paths'] as List?)?.cast<String>(); | ||
| if (paths != null) { | ||
| command.addAll(paths); |
There was a problem hiding this comment.
I don't think we should modify the input parameter like this - generally that isn't idiomatic dart style.
I would just copy the input list and then add the paths to the copy.
| /// Helper to run a command in multiple project roots. | ||
| /// | ||
| /// [defaultPaths] may be specified if one or more path arguments are required | ||
| /// for the command (e.g. `dart format <default paths>`). |
There was a problem hiding this comment.
I would mention here a bit more about the expected argument structure and the implicit handling of paths.
This refactors the pub support to be part of its own support mixin instead of being part of the Dart CLI tools mixin. This is because once #81 is resolved, this tool won't necessarily be using the Dart CLI or the Flutter CLI, but may use either.
This PR pulls out a utility method
runCommandInRootsfrom the Dart CLI support mixin for reuse.