list fixes when displaying help

This removes this --list option from dartfix and instead displays
the list of fixes when the --help option is specified.

Partially addresses #36875

Change-Id: Ibd8b124c2c20a752f7e661672eac8aa2e4483b26
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105002
Auto-Submit: Dan Rubel <danrubel@google.com>
Reviewed-by: Devon Carew <devoncarew@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>

pkg/dartfix/lib/src/driver.dart

@@ -36,7 +36,7 @@ class Driver {
     Context testContext,
     Logger testLogger,
   }) async {
-    final Options options = Options.parse(args);
+    final Options options = Options.parse(args, testContext, testLogger);
 
     force = options.force;
     overwrite = options.overwrite;
@@ -48,7 +48,7 @@ class Driver {
 
     // Start showing progress before we start the analysis server.
     Progress progress;
-    if (options.listFixes) {
+    if (options.showHelp) {
       progress = logger.progress('${ansi.emphasized('Listing fixes')}');
     } else {
       progress = logger.progress('${ansi.emphasized('Calculating fixes')}');
@@ -63,26 +63,26 @@ class Driver {
       context.exit(1);
     }
 
-    if (options.listFixes) {
+    if (options.showHelp) {
       try {
         await showListOfFixes(progress: progress);
       } finally {
         await server.stop();
       }
-    } else {
-      Future serverStopped;
+      context.exit(1);
+    }
 
-      try {
-        await setupFixesAnalysis(options);
-        result = await requestFixes(options, progress: progress);
-        serverStopped = server.stop();
-        await applyFixes();
-        await serverStopped;
-      } finally {
-        // If we didn't already try to stop the server, then stop it now.
-        if (serverStopped == null) {
-          await server.stop();
-        }
+    Future serverStopped;
+    try {
+      await setupFixesAnalysis(options);
+      result = await requestFixes(options, progress: progress);
+      serverStopped = server.stop();
+      await applyFixes();
+      await serverStopped;
+    } finally {
+      // If we didn't already try to stop the server, then stop it now.
+      if (serverStopped == null) {
+        await server.stop();
       }
     }
   }
@@ -122,14 +122,13 @@ class Driver {
       unsupportedOption(includeOption);
       return false;
     }
-    if (options.listFixes) {
-      unsupportedOption(listOption);
-      return false;
-    }
     if (options.requiredFixes) {
       unsupportedOption(requiredOption);
       return false;
     }
+    if (options.showHelp) {
+      return false;
+    }
     return true;
   }
 

pkg/dartfix/lib/src/options.dart

@@ -22,12 +22,12 @@ class Options {
   final List<String> excludeFixes;
 
   final bool force;
-  final bool listFixes;
+  final bool showHelp;
   final bool overwrite;
   final bool useColor;
   final bool verbose;
 
-  static Options parse(List<String> args, {Context context, Logger logger}) {
+  static Options parse(List<String> args, Context context, Logger logger) {
     final parser = new ArgParser(allowTrailingOptions: true)
       ..addSeparator('Choosing fixes to be applied:')
       ..addMultiOption(includeOption,
@@ -39,11 +39,6 @@ class Options {
           help: 'Apply required fixes.',
           defaultsTo: false,
           negatable: false)
-      ..addFlag(listOption,
-          abbr: 'l',
-          help: 'Display a list of fixes that can be applied.',
-          defaultsTo: false,
-          negatable: false)
       ..addSeparator('Modifying files:')
       ..addFlag(overwriteOption,
           abbr: 'w',
@@ -99,13 +94,9 @@ class Options {
     }
     options.logger = logger;
 
-    if (results[_helpOption] as bool) {
-      _showUsage(parser, logger);
-      context.exit(1);
-    }
-
-    // For '--list', we short circuit the logic to validate the sdk and project.
-    if (options.listFixes) {
+    // For '--help', we short circuit the logic to validate the sdk and project.
+    if (options.showHelp) {
+      _showUsage(parser, logger, showHelpHint: false);
       return options;
     }
 
@@ -157,10 +148,10 @@ class Options {
             (results[includeOption] as List ?? []).cast<String>().toList(),
         excludeFixes =
             (results[excludeOption] as List ?? []).cast<String>().toList(),
-        listFixes = results[listOption] as bool,
         overwrite = results[overwriteOption] as bool,
         requiredFixes = results[requiredOption] as bool,
         sdkPath = _getSdkPath(),
+        showHelp = results[_helpOption] as bool || results.arguments.isEmpty,
         targets = results.rest,
         useColor = results.wasParsed(_colorOption)
             ? results[_colorOption] as bool
@@ -181,7 +172,7 @@ class Options {
   }
 
   static _showUsage(ArgParser parser, Logger logger,
-      {bool showListHint = true}) {
+      {bool showHelpHint = true}) {
     logger.stderr('Usage: $_binaryName [options...] <directory paths>');
     logger.stderr('');
     logger.stderr(parser.usage);
@@ -190,10 +181,10 @@ class Options {
 If neither --$includeOption nor --$requiredOption is specified, then all fixes will be
 applied. Any fixes specified using --$excludeOption will not be applied regardless
 of whether they are required or specifed using --$includeOption.''');
-    if (showListHint) {
+    if (showHelpHint) {
       logger.stderr('''
 
-Use --list to display the fixes that can be specified using either
+Use --$_helpOption to display the fixes that can be specified using either
 --$includeOption or --$excludeOption.''');
     }
   }
@@ -209,5 +200,4 @@ const _verboseOption = 'verbose';
 // options only supported by server 1.22.2 and greater
 const excludeOption = 'exclude';
 const includeOption = 'include';
-const listOption = 'list';
 const requiredOption = 'required';

pkg/dartfix/test/all.dart

@@ -7,7 +7,7 @@ import 'package:test/test.dart';
 import 'src/client_version_test.dart' as client_version_test;
 import 'src/driver_exclude_test.dart' as driver_exclude_test;
 import 'src/driver_include_test.dart' as driver_include_test;
-import 'src/driver_list_test.dart' as driver_list_test;
+import 'src/driver_help_test.dart' as driver_list_test;
 import 'src/driver_required_test.dart' as driver_required_test;
 import 'src/driver_test.dart' as driver_test;
 import 'src/options_test.dart' as options_test;

pkg/dartfix/test/src/driver_help_test.dart

@@ -0,0 +1,55 @@
+// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:dartfix/src/driver.dart';
+import 'package:dartfix/src/options.dart';
+import 'package:test/test.dart';
+
+import 'test_context.dart';
+
+main() {
+  test('help explicit', () async {
+    final driver = new Driver();
+    final testContext = new TestContext();
+    final testLogger = new TestLogger();
+    try {
+      await driver.start(
+        ['--help'], // display help and list fixes
+        testContext: testContext,
+        testLogger: testLogger,
+      );
+      fail('expected exception');
+    } on TestExit catch (e) {
+      expect(e.code, 1);
+    }
+    final errText = testLogger.stderrBuffer.toString();
+    final outText = testLogger.stdoutBuffer.toString();
+    expect(errText, contains('--$excludeOption'));
+    expect(errText, isNot(contains('Use --help to display the fixes')));
+    expect(outText, contains('use-mixin'));
+  });
+
+  test('help implicit', () async {
+    final driver = new Driver();
+    final testContext = new TestContext();
+    final testLogger = new TestLogger();
+    try {
+      await driver.start(
+        [], // no options or arguments should display help and list fixes
+        testContext: testContext,
+        testLogger: testLogger,
+      );
+      fail('expected exception');
+    } on TestExit catch (e) {
+      expect(e.code, 1);
+    }
+    final errText = testLogger.stderrBuffer.toString();
+    final outText = testLogger.stdoutBuffer.toString();
+    print(errText);
+    print(outText);
+    expect(errText, contains('--$excludeOption'));
+    expect(errText, isNot(contains('Use --help to display the fixes')));
+    expect(outText, contains('use-mixin'));
+  });
+}

pkg/dartfix/test/src/options_test.dart

@@ -26,7 +26,7 @@ main() {
     bool force = false,
     List<String> includeFixes = const <String>[],
     List<String> excludeFixes = const <String>[],
-    bool listFixes = false,
+    bool showHelp = false,
     String normalOut,
     bool requiredFixes = false,
     bool overwrite = false,
@@ -36,7 +36,7 @@ main() {
     Options options;
     int actualExitCode;
     try {
-      options = Options.parse(args, context: context, logger: logger);
+      options = Options.parse(args, context, logger);
     } on TestExit catch (e) {
       actualExitCode = e.code;
     }
@@ -53,7 +53,7 @@ main() {
     expect(options.force, force);
     expect(options.requiredFixes, requiredFixes);
     expect(options.overwrite, overwrite);
-    expect(options.listFixes, listFixes);
+    expect(options.showHelp, showHelp);
     expect(options.includeFixes, includeFixes);
     expect(options.excludeFixes, excludeFixes);
     expect(options.verbose, verbose);
@@ -79,8 +79,12 @@ main() {
     parse(['--force', 'foo'], force: true, targetSuffixes: ['foo']);
   });
 
-  test('help', () {
-    parse(['--help'], errorOut: 'Display this help message', exitCode: 1);
+  test('help explicit', () {
+    parse(['--help'], errorOut: 'Display this help message', showHelp: true);
+  });
+
+  test('help implicit', () {
+    parse([], errorOut: 'Display this help message', showHelp: true);
   });
 
   test('include fix', () {
@@ -95,7 +99,7 @@ main() {
 
   test('invalid option no logger', () {
     try {
-      Options.parse(['--foo'], context: context);
+      Options.parse(['--foo'], context, null);
       fail('Expected exception');
     } on TestExit catch (e) {
       expect(e.code, 15, reason: 'exit code');
@@ -107,10 +111,6 @@ main() {
         errorOut: 'Expected directory, but found', exitCode: 15);
   });
 
-  test('list fixes', () {
-    parse(['--list'], normalOut: '', listFixes: true);
-  });
-
   test('overwrite', () {
     parse(['--overwrite', 'foo'], overwrite: true, targetSuffixes: ['foo']);
   });