fix(CLI): unified error handling

* Use `Result` everywhere, instead of Option or tuples
* Properly handle error occurring after the dry-run. We do it in an
  extensible way, in case we need to do more than handle invalid output
  files at some point. Output files that could not be opened will now
  result in a nice error message with all the information we have.

Fixes #66

Author: Byron

src/mako/cli/lib/engine.mako

@@ -36,6 +36,11 @@ use oauth2::{Authenticator, DefaultAuthenticatorDelegate};
 use serde::json;
 use clap::ArgMatches;
 
+enum DoitError {
+    IoError(String, io::Error),
+    ApiError(api::Error),
+}
+
 struct Engine<'n, 'a> {
     opt: ArgMatches<'n, 'a>,
     hub: ${hub_type_name}<hyper::Client, Authenticator<DefaultAuthenticatorDelegate, JsonTokenStorage, hyper::Client>>,
@@ -46,15 +51,15 @@ impl<'n, 'a> Engine<'n, 'a> {
 % for resource in sorted(c.rta_map.keys()):
     % for method in sorted(c.rta_map[resource]):
     fn ${call_method_ident(resource, method)}(&self, opt: &ArgMatches<'n, 'a>, dry_run: bool, err: &mut InvalidOptionsError)
-                                                    -> Option<api::Error> {
+                                                    -> Result<(), DoitError> {
         ${self._method_call_impl(c, resource, method) | indent_all_but_first_by(2)}
     }
 
     % endfor # each method
 % endfor 
-    fn _doit(&self, dry_run: bool) -> (Option<api::Error>, Option<InvalidOptionsError>) {
+    fn _doit(&self, dry_run: bool) -> Result<Result<(), DoitError>, Option<InvalidOptionsError>> {
         let mut err = InvalidOptionsError::new();
-        let mut call_result: Option<api::Error> = None;
+        let mut call_result: Result<(), DoitError> = Ok(());
         let mut err_opt: Option<InvalidOptionsError> = None;
 ## RESOURCE LOOP: check for set primary subcommand
         match ${SOPT + '.subcommand()'} {
@@ -83,8 +88,10 @@ impl<'n, 'a> Engine<'n, 'a> {
             if err.issues.len() > 0 {
                 err_opt = Some(err);
             }
+            Err(err_opt)
+        } else {
+            Ok(call_result)
         }
-        (call_result, err_opt)
     }
 
     // Please note that this call will fail if any part of the opt can't be handled
@@ -117,15 +124,17 @@ impl<'n, 'a> Engine<'n, 'a> {
         };
 
         match engine._doit(true) {
-            (_, Some(err)) => Err(err),
-            _ => Ok(engine),
+            Err(Some(err)) => Err(err),
+            Err(None)      => Ok(engine),
+            Ok(_)          => unreachable!(),
         }
     }
 
-    // Execute the call with all the bells and whistles, informing the caller only if there was an error.
-    // The absense of one indicates success.
-    fn doit(&self) -> Option<api::Error> {
-        self._doit(false).0
+    fn doit(&self) -> Result<(), DoitError> {
+        match self._doit(false) {
+            Ok(res) => res,
+            Err(_) => unreachable!(),
+        }
     }
 }
 </%def>
@@ -261,7 +270,7 @@ let mime_type = input_mime_from_opts(${opt_value(MIME_ARG, default=DEFAULT_MIME)
 let protocol = CallType::Standard;
 % endif # support upload
 if dry_run {
-    None
+    Ok(())
 } else {
     assert!(err.issues.len() == 0);
     % if method_default_scope(mc.m):
@@ -270,9 +279,11 @@ if dry_run {
     }
     % endif
     ## Make the call, handle uploads, handle downloads (also media downloads|json decoding)
-    ## TODO: unify error handling
     % if handle_output:
-    let mut ostream = writer_from_opts(opt.value_of("${(OUT_ARG)}"));
+    let mut ostream = match writer_from_opts(opt.value_of("${(OUT_ARG)}")) {
+        Ok(mut f) => f,
+        Err(io_err) => return Err(DoitError::IoError(${opt_value(OUT_ARG, default='-')}.to_string(), io_err)),
+    };
     % endif # handle output
     match match protocol {
         % if mc.media_params:
@@ -285,7 +296,7 @@ if dry_run {
         _ => unreachable!()
         % endif
     } {
-        Err(api_err) => Some(api_err),
+        Err(api_err) => Err(DoitError::ApiError(api_err)),
         % if mc.response_schema:
         Ok((mut response, output_schema)) => {
         % else:
@@ -310,7 +321,7 @@ if dry_run {
             % if track_download_flag:
             }
             % endif
-            None
+            Ok(())
         }
     }
 }\

src/mako/cli/main.rs.mako

@@ -4,7 +4,7 @@
 <%  
     from util import (new_context, rust_comment, to_extern_crate_name, library_to_crate_name, library_name,
                       indent_all_but_first_by)
-    from cli import DEBUG_FLAG    
+    from cli import OUT_ARG, DEBUG_FLAG, opt_value
 
     c = new_context(schemas, resources, context.get('methods'))
     default_user_agent = "google-cli-rust-client/" + cargo.build_version
@@ -38,17 +38,24 @@ fn main() {
     let debug = matches.is_present("${DEBUG_FLAG}");
     match Engine::new(matches) {
         Err(err) => {
-            writeln!(io::stderr(), "{}", err).ok();
             env::set_exit_status(err.exit_code);
+            writeln!(io::stderr(), "{}", err).ok();
         },
         Ok(engine) => {
-            if let Some(err) = engine.doit() {
-                if debug {
-                    writeln!(io::stderr(), "{:?}", err).ok();
-                } else {
-                    writeln!(io::stderr(), "{}", err).ok();
-                }
+            if let Err(doit_err) = engine.doit() {
                 env::set_exit_status(1);
+                match doit_err {
+                    DoitError::IoError(path, err) => {
+                        writeln!(io::stderr(), "Failed to open output file '{}': {}", path, err).ok();
+                    },
+                    DoitError::ApiError(err) => {
+                        if debug {
+                            writeln!(io::stderr(), "{:?}", err).ok();
+                        } else {
+                            writeln!(io::stderr(), "{}", err).ok();
+                        }
+                    }
+                }
             }
         }
     }

src/rust/cli/cmn.rs

@@ -169,13 +169,14 @@ pub fn input_mime_from_opts(mime: &str, err: &mut InvalidOptionsError) -> Option
     }
 }
 
-// May panic if we can't open the file - this is anticipated, we can't currently communicate this 
-// kind of error: TODO: fix this architecture :)
-pub fn writer_from_opts(arg: Option<&str>) -> Box<Write> {
+pub fn writer_from_opts(arg: Option<&str>) -> Result<Box<Write>, io::Error> {
     let f = arg.unwrap_or("-");
     match f {
-        "-" => Box::new(stdout()),
-        _ => Box::new(fs::OpenOptions::new().create(true).write(true).open(f).unwrap())
+        "-" => Ok(Box::new(stdout())),
+        _ => match fs::OpenOptions::new().create(true).write(true).open(f) {
+            Ok(f) => Ok(Box::new(f)),
+            Err(io_err) => Err(io_err),
+        }
     }
 }