Create CliCommand.dart that uses the Dart executable from flutter
#7085
Conversation
|
auto label is removed for flutter/devtools/7085, due to - The status or check suite Verify PR Release Note Requirements has failed. Please fix the issues identified (or deflake) before re-applying this label. |
DanTup
left a comment
There was a problem hiding this comment.
LGTM with a small nit I'll open a PR about.
| // the "dart devtools" command for testing local DevTools/server changes. | ||
| ...remainingArguments, | ||
| ], | ||
| ].join(' '), |
There was a problem hiding this comment.
It's not safe for us to combine these with spaces here because if serveLocalScriptPath or remainingArguments contain spaces, they'll be split into separate arguments. For example my Dart SDKs are in a folder named "Dart SDKs" on my Mac.
We've had some issues because of this in the past (like #6679).
I think we should remove all of the split(' ') code inside these helpers because it's too easy to accidentally pass things strings that had variables in that may or may not have spaces. If it's convenient to use strings for some things, I think we should add the .split(' ') in the call instead (so it's next to the string that may be using interpolated variables and much clearer that there may be a bug).
I'll open a PR to see what this looks like and discuss.
Fixes Dart-Code/Dart-Code#4953