From 2f200217f942aa0317186811dbbe95d675a17ab0 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 30 Apr 2015 16:31:49 +0200 Subject: [PATCH] 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 --- src/mako/cli/lib/engine.mako | 41 +++++++++++++++++++++++------------- src/mako/cli/main.rs.mako | 23 +++++++++++++------- src/rust/cli/cmn.rs | 11 +++++----- 3 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/mako/cli/lib/engine.mako b/src/mako/cli/lib/engine.mako index 10deae767f3..60797ada953 100644 --- a/src/mako/cli/lib/engine.mako +++ b/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}>, @@ -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 { + -> 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, Option) { + fn _doit(&self, dry_run: bool) -> Result, Option> { let mut err = InvalidOptionsError::new(); - let mut call_result: Option = None; + let mut call_result: Result<(), DoitError> = Ok(()); let mut err_opt: Option = 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 { - self._doit(false).0 + fn doit(&self) -> Result<(), DoitError> { + match self._doit(false) { + Ok(res) => res, + Err(_) => unreachable!(), + } } } @@ -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(()) } } }\ diff --git a/src/mako/cli/main.rs.mako b/src/mako/cli/main.rs.mako index 30a6689f12a..48b09593b16 100644 --- a/src/mako/cli/main.rs.mako +++ b/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(); + } + } + } } } } diff --git a/src/rust/cli/cmn.rs b/src/rust/cli/cmn.rs index c5ead901c33..f2a91ef4194 100644 --- a/src/rust/cli/cmn.rs +++ b/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 { +pub fn writer_from_opts(arg: Option<&str>) -> Result, 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), + } } }