Skip to content

some cleanup to the CLI UI #3129

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.

Sign up for GitHub

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

Merged
devoncarew merged 4 commits into from
Jun 18, 2021
Merged

some cleanup to the CLI UI #3129

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6ad52c
some cleanup to the CLI UI
devoncarew Jun 15, 2021
bf14615
fix a typo
devoncarew Jun 16, 2021
35096b4
add a todo link
devoncarew Jun 17, 2021
d98689b
Merge branch 'master' into cleanup_cli_args
devoncarew Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions packages/devtools_server/lib/src/server.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const argMachine = 'machine';
const argHost = 'host';
const argPort = 'port';
const argProfileMemory = 'record-memory-profile';
const argProfileMemoryOld = 'profile-memory';
const argTryPorts = 'try-ports';
const argVerbose = 'verbose';
const launchDevToolsService = 'launchDevTools';
Expand Down Expand Up @@ -119,8 +118,6 @@ Future<HttpServer?> _serveDevToolsWithArgs(
String? profileFilename;
if (args.wasParsed(argProfileMemory)) {
profileFilename = args[argProfileMemory];
} else if (args.wasParsed(argProfileMemoryOld)) {
profileFilename = args[argProfileMemoryOld];
}
if (profileFilename != null && !path.isAbsolute(profileFilename)) {
profileFilename = path.absolute(profileFilename);
Expand Down Expand Up @@ -498,7 +495,6 @@ ArgParser _createArgsParser(bool verbose) {
)
..addOption(
argHost,
//defaultsTo: 'localhost',
valueHelp: 'host',
help: 'Hostname to serve DevTools on (defaults to localhost).',
)
Expand All @@ -514,47 +510,49 @@ ArgParser _createArgsParser(bool verbose) {
help:
'Launches DevTools in a browser immediately at start.\n(defaults to on unless in --machine mode)',
)
// TODO: Remove this - prefer that clients use the rest arg.
..addOption(
argVmUri,
defaultsTo: '',
help: 'VM Service protocol URI.',
hide: true,
)
..addFlag(
argMachine,
negatable: false,
help: 'Sets output format to JSON for consumption in tools.',
)
);

parser.addSeparator('Memory profiling options:');

parser
..addOption(
argProfileMemory,
valueHelp: 'file',
defaultsTo: 'memory_samples.json',
help:
'Start devtools headlessly and write memory profiling samples to the '
'indicated file.',
)
// TODO: Remove this after a release or two.
..addOption(
argProfileMemoryOld,
defaultsTo: 'memory_samples.json',
hide: true,
)
);

if (verbose) {
parser.addSeparator('App size options:');
}

// TODO(devoncarew): --appSizeBase and --appSizeTest should be renamed to
// something like --app-size-base and --app-size-test; #3146.
parser
..addOption(
argAppSizeBase,

@devoncarew devoncarew Jun 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Longer term, we way want to deprecate --appSizeBase and --appSizeTest in favor of naming the options something like --app-size-base and --app-size-test.

@kenzieschmoll kenzieschmoll Jun 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind filing an issue for this and linking it in a TODO here?

@devoncarew devoncarew Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done! #3146

valueHelp: 'appSizeBase',
help: 'Path to the base app size file used for app size debugging.',
hide: true,
hide: !verbose,
)
..addOption(
argAppSizeTest,
valueHelp: 'appSizeTest',
help: 'Path to the test app size file used for app size debugging. This '
'file should only be specified if a base app size file (appSizeBase) '
'is also specified.',
hide: true,
help: 'Path to the test app size file used for app size debugging.\nThis '
'file should only be specified if --$argAppSizeBase is also specified.',
hide: !verbose,
);

if (verbose) {
parser.addSeparator('Advanced options:');
}

// Args to show for verbose mode.
parser
..addOption(
Expand Down Expand Up @@ -586,7 +584,17 @@ ArgParser _createArgsParser(bool verbose) {
hide: !verbose,
);

// Hidden args.
// Deprecated and hidden args.
// TODO: Remove this - prefer that clients use the rest arg.
parser
..addOption(
argVmUri,

@devoncarew devoncarew Jun 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know whether this can be removed yet? --vm-uri

@kenzieschmoll kenzieschmoll Jun 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacob314 do you know if this is used anywhere?

@jacob314 jacob314 Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just verified. It is not used in vscode, flutter-intellij or flutter.

defaultsTo: '',
help: 'VM Service protocol URI.',
hide: true,
);

// Development only args.
parser
..addFlag(
argDebugMode,
Expand Down