fixes #610, incorrect help output

bin/dartdevc.dart

Index: bin/dartdevc.dart
diff --git a/bin/dartdevc.dart b/bin/dartdevc.dart
index 98c3e450a2eb691b46021867f761740fa085c7f5..34b8c0c4dd6fba474d7836d7b6aa0978c53a524c 100755
--- a/bin/dartdevc.dart
+++ b/bin/dartdevc.dart
@@ -38,7 +38,6 @@
 import 'dart:async';
 import 'dart:io';
 import 'package:analyzer/src/generated/engine.dart' show AnalysisEngine;
-import 'package:args/command_runner.dart';
 import 'package:bazel_worker/bazel_worker.dart';
 import 'package:dev_compiler/src/compiler/command.dart';
 
@@ -49,26 +48,18 @@ Future main(List<String> args) async {
   if (args.contains('--persistent_worker')) {
     new _CompilerWorker(args..remove('--persistent_worker')).run();
   } else {
-    exitCode = await _runCommand(args);
+    exitCode = _runCommand(args);

[nweiz, 2016/08/12 20:30:57]

I'd personally be more thorough here and remove all the vestiges of the command
structure—make the code look like how you'd write it if you were writing a
non-command-based executable from the start. Probably just make CompileCommand a
single top-level function rather than a class, since it doesn't override
anything anymore.

[Jennifer Messerly, 2016/08/12 21:08:01]

yeah, it was kind of a convenient place to collect state, that's why I left it
(the whole objects-are-closures correspondence). but I went ahead and refactored
it away.

   }
 }
 
 /// Runs a single compile command, and returns an exit code.
-Future<int> _runCommand(List<String> args,
-    {MessageHandler messageHandler}) async {
-  try {
-    if (args.isEmpty || args.first != 'compile' && args.first != 'help') {
-      // TODO(jmesserly): we should deprecate the commands. For now they are
-      // still supported for backwards compatibility.
-      args.insert(0, 'compile');
-    }
-    var runner = new CommandRunner('dartdevc', 'Dart Development Compiler');
-    runner.addCommand(new CompileCommand(messageHandler: messageHandler));
-    await runner.run(args);
-  } catch (e, s) {
-    return _handleError(e, s, args, messageHandler: messageHandler);
+int _runCommand(List<String> args, {MessageHandler messageHandler}) {
+  // TODO(jmesserly): this is here for backwards compatibility, we should remove
+  // it at some point.

[nweiz, 2016/08/12 20:30:57]

Is it really important to be backwards compatible here? I doubt you have any
external users who are relying strongly on existing behavior, and IIRC most of
the time the command structure existed it didn't even work when invoked from the
SDK.

[Jennifer Messerly, 2016/08/12 21:08:01]

our internal build rules still pass it

[nweiz, 2016/08/12 21:16:37]

In that case, consider filing bugs to use the new API and linking those bugs
here.

[Jennifer Messerly, 2016/08/12 21:58:42]

CC'd you on my internal CL to fix it. I'm going to optimistically assume it
lands and remove this code :)

+  if (args.isNotEmpty && args[0] == 'compile') {
+    args.removeAt(0);
   }
-  return EXIT_CODE_OK;
+  return new CompileCommand(messageHandler: messageHandler).run(args);
 }
 
 /// Runs the compiler worker loop.
@@ -79,57 +70,15 @@ class _CompilerWorker extends AsyncWorkerLoop {
   _CompilerWorker(this._startupArgs) : super();
 
   /// Performs each individual work request.
-  Future<WorkResponse> performRequest(WorkRequest request) async {
+  Future<WorkResponse> performRequest(WorkRequest request) {
     var args = _startupArgs.toList()..addAll(request.arguments);
 
     var output = new StringBuffer();
-    var exitCode = await _runCommand(args, messageHandler: output.writeln);
+    var exitCode = _runCommand(args, messageHandler: output.writeln);
     AnalysisEngine.instance.clearCaches();
-    return new WorkResponse()
+    return new Future.value(new WorkResponse()

[nweiz, 2016/08/12 20:30:57]

Nit, but if a function doesn't do any async work but has to return a future to
fit an interface, I usually annotate it "async" rather than using "new
Future.value". That way you can refactor the function internals all you want
without messing with the semantics.

[Jennifer Messerly, 2016/08/12 21:58:42]

Done.

       ..exitCode = exitCode
-      ..output = output.toString();
-  }
-}
-
-/// Handles [error] in a uniform fashion. Returns the proper exit code and calls
-/// [messageHandler] with messages.
-int _handleError(dynamic error, dynamic stackTrace, List<String> args,
-    {MessageHandler messageHandler}) {
-  messageHandler ??= print;
-
-  if (error is UsageException) {
-    // Incorrect usage, input file not found, etc.
-    messageHandler(error);
-    return 64;
-  } else if (error is CompileErrorException) {
-    // Code has error(s) and failed to compile.
-    messageHandler(error);
-    return 1;
-  } else {
-    // Anything else is likely a compiler bug.
-    //
-    // --unsafe-force-compile is a bit of a grey area, but it's nice not to
-    // crash while compiling
-    // (of course, output code may crash, if it had errors).
-    //
-    messageHandler("");
-    messageHandler("We're sorry, you've found a bug in our compiler.");
-    messageHandler("You can report this bug at:");
-    messageHandler("    https://github.com/dart-lang/dev_compiler/issues");
-    messageHandler("");
-    messageHandler(
-        "Please include the information below in your report, along with");
-    messageHandler(
-        "any other information that may help us track it down. Thanks!");
-    messageHandler("");
-    messageHandler("    dartdevc arguments: " + args.join(' '));
-    messageHandler("    dart --version: ${Platform.version}");
-    messageHandler("");
-    messageHandler("```");
-    messageHandler(error);
-    messageHandler(stackTrace);
-    messageHandler("```");
-    return 70;
+      ..output = output.toString());
   }
 }