From e5414695d4e429aff3e7756663dc16d6d7faa6b8 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Fri, 23 Apr 2021 16:29:38 -0700 Subject: [PATCH] Change --disable-dds to --no-dds to avoid double negatives (#80900) Also, refactor internal code to do the same. See https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-double-negatives-in-apis --- .../lib/src/commands/attach.dart | 2 +- .../lib/src/commands/devices.dart | 2 +- .../flutter_tools/lib/src/commands/run.dart | 2 +- .../flutter_tools/lib/src/commands/test.dart | 2 +- packages/flutter_tools/lib/src/device.dart | 6 +- .../lib/src/isolated/resident_web_runner.dart | 2 +- .../lib/src/resident_runner.dart | 8 +-- .../lib/src/runner/flutter_command.dart | 69 ++++++++++++++----- .../lib/src/test/flutter_tester_device.dart | 4 +- .../test/general.shard/cold_test.dart | 2 +- .../drive/drive_service_test.dart | 2 +- .../flutter_tester_device_test.dart | 2 +- .../test/general.shard/hot_test.dart | 2 +- .../general.shard/resident_runner_test.dart | 2 +- .../runner/flutter_command_test.dart | 62 ++++++++++++++++- .../test/integration.shard/test_driver.dart | 4 +- 16 files changed, 132 insertions(+), 41 deletions(-) diff --git a/packages/flutter_tools/lib/src/commands/attach.dart b/packages/flutter_tools/lib/src/commands/attach.dart index 5cd474939e..0ad846e91f 100644 --- a/packages/flutter_tools/lib/src/commands/attach.dart +++ b/packages/flutter_tools/lib/src/commands/attach.dart @@ -430,7 +430,7 @@ known, it can be explicitly provided to attach via the command-line, e.g. final List flutterDevices = [flutterDevice]; final DebuggingOptions debuggingOptions = DebuggingOptions.enabled( buildInfo, - disableDds: boolArg('disable-dds'), + enableDds: enableDds, devToolsServerAddress: devToolsServerAddress, ); diff --git a/packages/flutter_tools/lib/src/commands/devices.dart b/packages/flutter_tools/lib/src/commands/devices.dart index afe635bcca..ebef8e78b1 100644 --- a/packages/flutter_tools/lib/src/commands/devices.dart +++ b/packages/flutter_tools/lib/src/commands/devices.dart @@ -48,7 +48,7 @@ class DevicesCommand extends FlutterCommand { @override Future validateCommand() { if (argResults['timeout'] != null) { - globals.printError('"--timeout" argument is deprecated, use "--${FlutterOptions.kDeviceTimeout}" instead'); + globals.printError('${globals.logger.terminal.warningMark} The "--timeout" argument is deprecated; use "--${FlutterOptions.kDeviceTimeout}" instead.'); } return super.validateCommand(); } diff --git a/packages/flutter_tools/lib/src/commands/run.dart b/packages/flutter_tools/lib/src/commands/run.dart index 9506931b77..c66ca4b903 100644 --- a/packages/flutter_tools/lib/src/commands/run.dart +++ b/packages/flutter_tools/lib/src/commands/run.dart @@ -191,7 +191,7 @@ abstract class RunCommandBase extends FlutterCommand with DeviceBasedDevelopment buildInfo, startPaused: boolArg('start-paused'), disableServiceAuthCodes: boolArg('disable-service-auth-codes'), - disableDds: boolArg('disable-dds'), + enableDds: enableDds, dartEntrypointArgs: stringsArg('dart-entrypoint-args'), dartFlags: stringArg('dart-flags') ?? '', useTestFonts: argParser.options.containsKey('use-test-fonts') && boolArg('use-test-fonts'), diff --git a/packages/flutter_tools/lib/src/commands/test.dart b/packages/flutter_tools/lib/src/commands/test.dart index 320b59b86d..c1f3dc6dcb 100644 --- a/packages/flutter_tools/lib/src/commands/test.dart +++ b/packages/flutter_tools/lib/src/commands/test.dart @@ -392,7 +392,7 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts { disableServiceAuthCodes: boolArg('disable-service-auth-codes'), // On iOS >=14, keeping this enabled will leave a prompt on the screen. disablePortPublication: true, - disableDds: disableDds, + enableDds: enableDds, nullAssertions: boolArg(FlutterOptions.kNullAssertions), ); diff --git a/packages/flutter_tools/lib/src/device.dart b/packages/flutter_tools/lib/src/device.dart index c50dc2822f..44247c0a27 100644 --- a/packages/flutter_tools/lib/src/device.dart +++ b/packages/flutter_tools/lib/src/device.dart @@ -717,7 +717,7 @@ class DebuggingOptions { this.buildInfo, { this.startPaused = false, this.disableServiceAuthCodes = false, - this.disableDds = false, + this.enableDds = true, this.dartEntrypointArgs = const [], this.dartFlags = '', this.enableSoftwareRendering = false, @@ -768,7 +768,7 @@ class DebuggingOptions { startPaused = false, dartFlags = '', disableServiceAuthCodes = false, - disableDds = false, + enableDds = true, enableSoftwareRendering = false, skiaDeterministicRendering = false, traceSkia = false, @@ -795,7 +795,7 @@ class DebuggingOptions { final String dartFlags; final List dartEntrypointArgs; final bool disableServiceAuthCodes; - final bool disableDds; + final bool enableDds; final bool enableSoftwareRendering; final bool skiaDeterministicRendering; final bool traceSkia; diff --git a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart index b390ecef2d..956f397254 100644 --- a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart +++ b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart @@ -270,7 +270,7 @@ class ResidentWebRunner extends ResidentRunner { useSseForInjectedClient: debuggingOptions.webUseSseForInjectedClient, buildInfo: debuggingOptions.buildInfo, enableDwds: _enableDwds, - enableDds: !debuggingOptions.disableDds, + enableDds: debuggingOptions.enableDds, entrypoint: _fileSystem.file(target).uri, expressionCompiler: expressionCompiler, chromiumLauncher: _chromiumLauncher, diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 399d9dead2..0342a56138 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -231,7 +231,7 @@ class FlutterDevice { int hostVmServicePort, int ddsPort, bool disableServiceAuthCodes = false, - bool disableDds = false, + bool enableDds = true, @required bool allowExistingDdsInstance, bool ipv6 = false, }) { @@ -245,7 +245,7 @@ class FlutterDevice { isWaitingForVm = true; bool existingDds = false; FlutterVmService service; - if (!disableDds) { + if (enableDds) { void handleError(Exception e, StackTrace st) { globals.printTrace('Fail to connect to service protocol: $observatoryUri: $e'); if (!completer.isCompleted) { @@ -300,7 +300,7 @@ class FlutterDevice { service = await Future.any( >[ connectToVmService( - disableDds ? observatoryUri : device.dds.uri, + enableDds ? device.dds.uri : observatoryUri, reloadSources: reloadSources, restart: restart, compileExpression: compileExpression, @@ -1338,7 +1338,7 @@ abstract class ResidentRunner extends ResidentHandlers { reloadSources: reloadSources, restart: restart, compileExpression: compileExpression, - disableDds: debuggingOptions.disableDds, + enableDds: debuggingOptions.enableDds, ddsPort: debuggingOptions.ddsPort, allowExistingDdsInstance: allowExistingDdsInstance, hostVmServicePort: debuggingOptions.hostVmServicePort, diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart index 59deb601d5..685e6e7d6f 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -370,18 +370,46 @@ abstract class FlutterCommand extends Command { 'Specifying port 0 (the default) will find a random free port.' ); argParser.addFlag( - 'disable-dds', // TODO(ianh): this should be called `dds` and default to true (see style guide about double negatives) + 'dds', hide: !verboseHelp, - help: 'Disable the Dart Developer Service (DDS). This flag should only be provided ' - 'when attaching to an application with an existing DDS instance (e.g., ' - 'attaching to an application currently connected to by "flutter run") or ' - 'when running certain tests.\n' - 'Passing this flag may degrade IDE functionality if a DDS instance is not ' - 'already connected to the target application.' + defaultsTo: true, + help: 'Enable the Dart Developer Service (DDS).\n' + 'It may be necessary to disable this when attaching to an application with ' + 'an existing DDS instance (e.g., attaching to an application currently ' + 'connected to by "flutter run"), or when running certain tests.\n' + 'Disabling this feature may degrade IDE functionality if a DDS instance is ' + 'not already connected to the target application.' + ); + argParser.addFlag( + 'disable-dds', + hide: !verboseHelp, + help: '(deprecated; use "--no-dds" instead) ' + 'Disable the Dart Developer Service (DDS).' ); } - bool get disableDds => boolArg('disable-dds'); + bool _ddsEnabled; + bool get enableDds { + if (_ddsEnabled == null) { + if (argResults.wasParsed('disable-dds')) { + if (argResults.wasParsed('dds')) { + throwToolExit('The "--[no-]dds" and "--[no-]disable-dds" arguments are mutually exclusive. Only specify "--[no-]dds".'); + } + _ddsEnabled = !boolArg('disable-dds'); + // TODO(ianh): enable the following code once google3 is migrated away from --disable-dds (and add test to flutter_command_test.dart) + if (false) { // ignore: dead_code + if (_ddsEnabled) { + globals.printError('${globals.logger.terminal.warningMark} The "--no-disable-dds" argument is deprecated and redundant, and should be omitted.'); + } else { + globals.printError('${globals.logger.terminal.warningMark} The "--disable-dds" argument is deprecated. Use "--no-dds" instead.'); + } + } + } else { + _ddsEnabled = boolArg('dds'); + } + } + return _ddsEnabled; + } bool get _hostVmServicePortProvided => argResults.wasParsed('observatory-port') || argResults.wasParsed('host-vmservice-port'); @@ -435,7 +463,7 @@ abstract class FlutterCommand extends Command { // If DDS is enabled and no explicit DDS port is provided, use the // host-vmservice-port for DDS instead and bind the VM service to a random // port. - if (!disableDds && !argResults.wasParsed('dds-port')) { + if (enableDds && !argResults.wasParsed('dds-port')) { return null; } return _tryParseHostVmservicePort(); @@ -457,11 +485,12 @@ abstract class FlutterCommand extends Command { void addPublishPort({ bool enabledByDefault = true, bool verboseHelp = false }) { argParser.addFlag('publish-port', - negatable: true, - hide: !verboseHelp, - help: 'Publish the VM service port over mDNS. Disable to prevent the ' - 'local network permission app dialog in debug and profile build modes (iOS devices only.)', - defaultsTo: enabledByDefault); + negatable: true, + hide: !verboseHelp, + help: 'Publish the VM service port over mDNS. Disable to prevent the ' + 'local network permission app dialog in debug and profile build modes (iOS devices only).', + defaultsTo: enabledByDefault, + ); } bool get disablePortPublication => !boolArg('publish-port'); @@ -1068,11 +1097,13 @@ abstract class FlutterCommand extends Command { void _printDeprecationWarning() { if (deprecated) { - globals.printStatus('${globals.logger.terminal.warningMark} The "$name" command is deprecated and ' - 'will be removed in a future version of Flutter. ' - 'See https://flutter.dev/docs/development/tools/sdk/releases ' - 'for previous releases of Flutter.'); - globals.printStatus(''); + globals.printError( + '${globals.logger.terminal.warningMark} The "$name" command is deprecated and ' + 'will be removed in a future version of Flutter. ' + 'See https://flutter.dev/docs/development/tools/sdk/releases ' + 'for previous releases of Flutter.', + ); + globals.printError(''); } } diff --git a/packages/flutter_tools/lib/src/test/flutter_tester_device.dart b/packages/flutter_tools/lib/src/test/flutter_tester_device.dart index 0c53937e57..82fcccb4b1 100644 --- a/packages/flutter_tools/lib/src/test/flutter_tester_device.dart +++ b/packages/flutter_tools/lib/src/test/flutter_tester_device.dart @@ -98,7 +98,7 @@ class FlutterTesterTestDevice extends TestDevice { // // I mention this only so that you won't be tempted, as I was, to apply // the obvious simplification to this code and remove this entire feature. - '--observatory-port=${debuggingOptions.disableDds ? debuggingOptions.hostVmServicePort: 0}', + '--observatory-port=${debuggingOptions.enableDds ? 0 : debuggingOptions.hostVmServicePort }', if (debuggingOptions.startPaused) '--start-paused', if (debuggingOptions.disableServiceAuthCodes) '--disable-service-auth-codes', ] @@ -158,7 +158,7 @@ class FlutterTesterTestDevice extends TestDevice { debuggingOptions.hostVmServicePort == detectedUri.port); Uri forwardingUri; - if (!debuggingOptions.disableDds) { + if (debuggingOptions.enableDds) { logger.printTrace('test $id: Starting Dart Development Service'); final DartDevelopmentService dds = await startDds(detectedUri); forwardingUri = dds.uri; diff --git a/packages/flutter_tools/test/general.shard/cold_test.dart b/packages/flutter_tools/test/general.shard/cold_test.dart index d2587aa8de..da232531a8 100644 --- a/packages/flutter_tools/test/general.shard/cold_test.dart +++ b/packages/flutter_tools/test/general.shard/cold_test.dart @@ -206,7 +206,7 @@ class TestFlutterDevice extends FlutterDevice { CompileExpression compileExpression, GetSkSLMethod getSkSLMethod, PrintStructuredErrorLogMethod printStructuredErrorLogMethod, - bool disableDds = false, + bool enableDds = true, bool disableServiceAuthCodes = false, int hostVmServicePort, int ddsPort, diff --git a/packages/flutter_tools/test/general.shard/drive/drive_service_test.dart b/packages/flutter_tools/test/general.shard/drive/drive_service_test.dart index 97098f47b4..9b1c80afb1 100644 --- a/packages/flutter_tools/test/general.shard/drive/drive_service_test.dart +++ b/packages/flutter_tools/test/general.shard/drive/drive_service_test.dart @@ -231,7 +231,7 @@ void main() { observatoryUri: Uri.parse('http://127.0.0.1:63426/1UasC_ihpXY=/'), )); - await driverService.start(BuildInfo.profile, device, DebuggingOptions.enabled(BuildInfo.profile, disableDds: true), true); + await driverService.start(BuildInfo.profile, device, DebuggingOptions.enabled(BuildInfo.profile, enableDds: false), true); final int testResult = await driverService.startTest( 'foo.test', [], diff --git a/packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart b/packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart index e107e08827..e5226fb5de 100644 --- a/packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart +++ b/packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart @@ -221,7 +221,7 @@ class TestFlutterTesterDevice extends FlutterTesterTestDevice { packagesPath: '.dart_tool/package_config.json', ), startPaused: false, - disableDds: false, + enableDds: true, disableServiceAuthCodes: false, hostVmServicePort: 1234, nullAssertions: false, diff --git a/packages/flutter_tools/test/general.shard/hot_test.dart b/packages/flutter_tools/test/general.shard/hot_test.dart index e6c22f130b..3572e865af 100644 --- a/packages/flutter_tools/test/general.shard/hot_test.dart +++ b/packages/flutter_tools/test/general.shard/hot_test.dart @@ -366,7 +366,7 @@ class TestFlutterDevice extends FlutterDevice { GetSkSLMethod getSkSLMethod, PrintStructuredErrorLogMethod printStructuredErrorLogMethod, bool disableServiceAuthCodes = false, - bool disableDds = false, + bool enableDds = true, bool ipv6 = false, int hostVmServicePort, int ddsPort, diff --git a/packages/flutter_tools/test/general.shard/resident_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_runner_test.dart index d7c6cb2f95..94c9b42c6c 100644 --- a/packages/flutter_tools/test/general.shard/resident_runner_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_runner_test.dart @@ -2517,7 +2517,7 @@ class FakeFlutterDevice extends FlutterDevice { Future connect({ ReloadSources reloadSources, Restart restart, - bool disableDds = false, + bool enableDds = true, bool disableServiceAuthCodes = false, bool ipv6 = false, CompileExpression compileExpression, diff --git a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart index 8f1222a200..c93e012655 100644 --- a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart @@ -87,7 +87,7 @@ void main() { final CommandRunner runner = createTestCommandRunner(flutterCommand); await runner.run(['deprecated']); - expect(testLogger.statusText, + expect(testLogger.errorText, contains('The "deprecated" command is deprecated and will be removed in ' 'a future version of Flutter.')); expect(flutterCommand.usage, @@ -546,6 +546,49 @@ void main() { final BuildInfo buildInfo = await flutterCommand.getBuildInfo(forcedBuildMode: BuildMode.debug); expect(buildInfo.packagesPath, 'foo'); }); + + testUsingContext('dds options', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--dds-port=1']); + expect(ddsCommand.enableDds, isTrue); + expect(ddsCommand.ddsPort, 1); + }); + + testUsingContext('dds options --dds', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--dds']); + expect(ddsCommand.enableDds, isTrue); + }); + + testUsingContext('dds options --no-dds', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--no-dds']); + expect(ddsCommand.enableDds, isFalse); + }); + + testUsingContext('dds options --disable-dds', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--disable-dds']); + expect(ddsCommand.enableDds, isFalse); + }); + + testUsingContext('dds options --no-disable-dds', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--no-disable-dds']); + expect(ddsCommand.enableDds, isTrue); + }); + + testUsingContext('dds options --dds --disable-dds', () async { + final FakeDdsCommand ddsCommand = FakeDdsCommand(); + final CommandRunner runner = createTestCommandRunner(ddsCommand); + await runner.run(['test', '--dds', '--disable-dds']); + expect(() => ddsCommand.enableDds, throwsToolExit()); + }); }); } @@ -611,6 +654,23 @@ class FakeReportingNullSafetyCommand extends FlutterCommand { } } +class FakeDdsCommand extends FlutterCommand { + FakeDdsCommand() { + addDdsOptions(verboseHelp: false); + } + + @override + String get description => 'test'; + + @override + String get name => 'test'; + + @override + Future runCommand() async { + return FlutterCommandResult.success(); + } +} + class MockProcessInfo extends Mock implements ProcessInfo {} class MockIoProcessSignal extends Mock implements io.ProcessSignal {} diff --git a/packages/flutter_tools/test/integration.shard/test_driver.dart b/packages/flutter_tools/test/integration.shard/test_driver.dart index 1cad0ca380..44414c3e83 100644 --- a/packages/flutter_tools/test/integration.shard/test_driver.dart +++ b/packages/flutter_tools/test/integration.shard/test_driver.dart @@ -474,7 +474,7 @@ class FlutterRunTestDriver extends FlutterTestDriver { if (!chrome) '--disable-service-auth-codes', '--machine', - if (!spawnDdsInstance) '--disable-dds', + if (!spawnDdsInstance) '--no-dds', ...getLocalEngineArguments(), '-d', if (chrome) @@ -512,7 +512,7 @@ class FlutterRunTestDriver extends FlutterTestDriver { ...getLocalEngineArguments(), '--machine', if (!spawnDdsInstance) - '--disable-dds', + '--no-dds', '-d', 'flutter-tester', '--debug-port',