Convert more pub code to use async/await.

sdk/lib/_internal/pub/bin/pub.dart

Index: sdk/lib/_internal/pub/bin/pub.dart
diff --git a/sdk/lib/_internal/pub/bin/pub.dart b/sdk/lib/_internal/pub/bin/pub.dart
index 8f7fa4e32ddb192e82476c190c4953617b49973d..46f5b712d13c399b1174f0e1f9bc0262bddbf403 100644
--- a/sdk/lib/_internal/pub/bin/pub.dart
+++ b/sdk/lib/_internal/pub/bin/pub.dart
@@ -129,7 +129,7 @@ int chooseExitCode(exception) {
 }
 
 /// Walks the command tree and runs the selected pub command.
-Future invokeCommand(String cacheDir, ArgResults mainOptions) {
+Future invokeCommand(String cacheDir, ArgResults mainOptions) async {

[nweiz, 2014/08/29 22:27:39]

I think we should omit the return type annotation for async methods that
conceptually return "Future<void>" for the same reason we omit the return type
annotation for setters: the return type is already implicit in the signature,
and writing it down provides no additional value.

[Bob Nystrom, 2014/08/29 23:03:20]

Actually, that's not why we omit it for setters. The return type of a setter is
always *void*, not inferred from the parameter type. There are only two allowed
return types for a setter: "void" or nothing. Anything else produces a warning.

A set expression invokes the setter, but does not return the result of the
setter. It returns the RHS. The return value of the setter method is discarded.

In this case, I think we should keep the return type as Future. The fact that
the body uses async is an implementation detail. It won't be (and shouldn't be)
processed by stuff like dartdoc. If we get a warning if you call a function that
returns a Future in statement position (which I think is very important), that
warning will be based on the annotated type, and won't take the async modifier
into account. (I realize that the warning *could* take that into account, but
good luck selling the language team on that.)

I think it's best to think of "async" as an implementation detail of the body. A
method like { return 123; } also has a trivially inferrable type, but we don't
mind annotating that.

[nweiz, 2014/08/29 23:30:41]

Despite the fact that the parser considers "async" to be attached to the body,
it looks like it's part of the signature and a human reader is going to read it
as such. We should optimize for humans first and let the tools follow second,
and to any human it's clear from nothing more than the declaration line of the
function that it's going to return a future whether it's annotated or not.

[Bob Nystrom, 2014/09/03 23:44:16]

I don't think it's part of the signature and I'm quite certain our tools won't
be interpreting it as such. Gilad has been very clear in multiple places that
"async" is an implementation detail and is unrelated to the static return type.

What I can do is change this to Future<Null>, which makes it clear it doesn't
complete to a value. Thoughts?

[nweiz, 2014/09/05 18:57:38]

I'm not saying it's literally part of the signature, I'm saying it looks like it
is and that's what matters for reading the code. Gilad can say whatever he wants
but ultimately readers of the code read the syntax and not his emails.

That said, I don't want to block the patch on this debate. If you feel strongly
that we should include both declarations, go for it.
I think just "Future" is redundant; I don't want to add *more* text.

   var commands = PubCommand.mainCommands;
   var command;
   var commandString = "pub";
@@ -174,26 +174,23 @@ Future invokeCommand(String cacheDir, ArgResults mainOptions) {
         'Command "${options.name}" does not take any arguments.');
   }
 
-  return syncFuture(() {
+  try {
     return command.run(cacheDir, mainOptions, options);

[nweiz, 2014/08/29 22:27:39]

I also think we should prefer "await" to "return" if we aren't actually
returning a value. Not only does that better communicate that the return value
of e.g. command.run is unused, it also makes it easier to add additional logic
later in the function.

[Bob Nystrom, 2014/08/29 23:03:20]

Done and agreed. When converting the vanilla code over, it isn't always easy to
see which calls were producing values, so I got some places wrong.

-  }).whenComplete(() {
+  } finally {
     command.cache.deleteTempDir();
-  });
+  }
 }
 
 /// Checks that pub is running on a supported platform.
 ///
 /// If it isn't, it prints an error message and exits. Completes when the
 /// validation is done.
-Future validatePlatform() {
-  return syncFuture(() {
-    if (Platform.operatingSystem != 'windows') return null;
-
-    return runProcess('ver', []).then((result) {
-      if (result.stdout.join('\n').contains('XP')) {
-        log.error('Sorry, but pub is not supported on Windows XP.');
-        return flushThenExit(exit_codes.USAGE);
-      }
-    });
-  });
+Future validatePlatform() async {
+  if (Platform.operatingSystem != 'windows') return;
+
+  var result = await runProcess('ver', []);
+  if (result.stdout.join('\n').contains('XP')) {
+    log.error('Sorry, but pub is not supported on Windows XP.');
+    return flushThenExit(exit_codes.USAGE);
+  }
 }