From 54be5bf16e494227425fa66bae64f9c1cc58fcc4 Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Sun, 16 Jun 2019 06:36:17 +1200 Subject: [PATCH] Update errors and improve ctrl-c --- src/cli.rs | 75 +++++++++++++++++++++----------------- src/commands/cd.rs | 16 +++++--- src/commands/classified.rs | 32 ---------------- src/commands/enter.rs | 6 ++- src/commands/first.rs | 14 +++---- src/commands/from_xml.rs | 13 +++++-- src/commands/get.rs | 16 +++----- src/commands/ls.rs | 6 ++- src/commands/open.rs | 6 ++- src/commands/pick.rs | 14 +++---- src/commands/reject.rs | 14 +++---- src/commands/save.rs | 6 ++- src/commands/size.rs | 8 +++- src/commands/skip.rs | 14 +++---- src/commands/view.rs | 14 +++---- src/commands/where_.rs | 8 +++- src/errors.rs | 14 +++++++ 17 files changed, 139 insertions(+), 137 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 4c631a619..8f0027fe0 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -101,13 +101,10 @@ pub async fn cli() -> Result<(), Box> { cc.store(true, Ordering::SeqCst); }) .expect("Error setting Ctrl-C handler"); - + let mut ctrlcbreak = false; loop { if ctrl_c.load(Ordering::SeqCst) { ctrl_c.store(false, Ordering::SeqCst); - if let ShellError::String(s) = ShellError::string("CTRL-C") { - context.host.lock().unwrap().stdout(&format!("{:?}", s)); - } continue; } @@ -133,6 +130,20 @@ pub async fn cli() -> Result<(), Box> { rl.add_history_entry(line.clone()); } + LineResult::CtrlC => { + if ctrlcbreak { + std::process::exit(0); + } else { + context + .host + .lock() + .unwrap() + .stdout("CTRL-C pressed (again to quit)"); + ctrlcbreak = true; + continue; + } + } + LineResult::Error(mut line, err) => match err { ShellError::Diagnostic(diag) => { let host = context.host.lock().unwrap(); @@ -176,6 +187,7 @@ pub async fn cli() -> Result<(), Box> { .stdout(&format!("A surprising fatal error occurred.\n{:?}", err)); } } + ctrlcbreak = false; } rl.save_history("history.txt").unwrap(); @@ -185,6 +197,7 @@ pub async fn cli() -> Result<(), Box> { enum LineResult { Success(String), Error(String, ShellError), + CtrlC, Break, #[allow(unused)] @@ -200,6 +213,7 @@ impl std::ops::Try for LineResult { LineResult::Success(s) => Ok(Some(s)), LineResult::Error(_, s) => Err(s), LineResult::Break => Ok(None), + LineResult::CtrlC => Ok(None), LineResult::FatalError(err) => Err(err), } } @@ -258,27 +272,26 @@ async fn process_line(readline: Result, ctx: &mut Context (None, _) => break, (Some(ClassifiedCommand::Expr(_)), _) => { - return LineResult::Error(line.clone(), ShellError::unimplemented( - "Expression-only commands", - )) + return LineResult::Error( + line.clone(), + ShellError::unimplemented("Expression-only commands"), + ) } (_, Some(ClassifiedCommand::Expr(_))) => { - return LineResult::Error(line.clone(), ShellError::unimplemented( - "Expression-only commands", - )) + return LineResult::Error( + line.clone(), + ShellError::unimplemented("Expression-only commands"), + ) } - (Some(ClassifiedCommand::Sink(_)), Some(_)) => { - return LineResult::Error(line.clone(), ShellError::string("Commands like table, save, and autoview must come last in the pipeline")) + (Some(ClassifiedCommand::Sink(SinkCommand { name_span, .. })), Some(_)) => { + return LineResult::Error(line.clone(), ShellError::maybe_labeled_error("Commands like table, save, and autoview must come last in the pipeline", "must come last", name_span)); } (Some(ClassifiedCommand::Sink(left)), None) => { let input_vec: Vec = input.objects.collect().await; - left.run( - ctx, - input_vec, - )?; + left.run(ctx, input_vec)?; break; } @@ -290,13 +303,12 @@ async fn process_line(readline: Result, ctx: &mut Context Err(err) => return LineResult::Error(line.clone(), err), }, - ( - Some(ClassifiedCommand::Internal(left)), - Some(_), - ) => match left.run(ctx, input).await { - Ok(val) => ClassifiedInputStream::from_input_stream(val), - Err(err) => return LineResult::Error(line.clone(), err), - }, + (Some(ClassifiedCommand::Internal(left)), Some(_)) => { + match left.run(ctx, input).await { + Ok(val) => ClassifiedInputStream::from_input_stream(val), + Err(err) => return LineResult::Error(line.clone(), err), + } + } (Some(ClassifiedCommand::Internal(left)), None) => { match left.run(ctx, input).await { @@ -313,13 +325,12 @@ async fn process_line(readline: Result, ctx: &mut Context Err(err) => return LineResult::Error(line.clone(), err), }, - ( - Some(ClassifiedCommand::External(left)), - Some(_), - ) => match left.run(ctx, input, StreamNext::Internal).await { - Ok(val) => val, - Err(err) => return LineResult::Error(line.clone(), err), - }, + (Some(ClassifiedCommand::External(left)), Some(_)) => { + match left.run(ctx, input, StreamNext::Internal).await { + Ok(val) => val, + Err(err) => return LineResult::Error(line.clone(), err), + } + } (Some(ClassifiedCommand::External(left)), None) => { match left.run(ctx, input, StreamNext::Last).await { @@ -332,9 +343,7 @@ async fn process_line(readline: Result, ctx: &mut Context LineResult::Success(line.clone()) } - Err(ReadlineError::Interrupted) => { - LineResult::Error("".to_string(), ShellError::string("CTRL-C")) - } + Err(ReadlineError::Interrupted) => LineResult::CtrlC, Err(ReadlineError::Eof) => { println!("CTRL-D"); LineResult::Break diff --git a/src/commands/cd.rs b/src/commands/cd.rs index 8716a4362..380c7d583 100644 --- a/src/commands/cd.rs +++ b/src/commands/cd.rs @@ -13,17 +13,23 @@ pub fn cd(args: CommandArgs) -> Result { let path = match args.positional.first() { None => match dirs::home_dir() { Some(o) => o, - _ => return Err(ShellError::string("Can not change to home directory")), + _ => { + return Err(ShellError::maybe_labeled_error( + "Can not change to home directory", + "can not go to home", + args.name_span, + )) + } }, Some(v) => { let target = v.as_string()?.clone(); match dunce::canonicalize(cwd.join(&target).as_path()) { Ok(p) => p, Err(_) => { - return Err(ShellError::labeled_error( + return Err(ShellError::maybe_labeled_error( "Can not change to directory", "directory not found", - args.positional[0].span.clone(), + Some(args.positional[0].span.clone()), )); } } @@ -35,10 +41,10 @@ pub fn cd(args: CommandArgs) -> Result { Ok(_) => {} Err(_) => { if args.positional.len() > 0 { - return Err(ShellError::labeled_error( + return Err(ShellError::maybe_labeled_error( "Can not change to directory", "directory not found", - args.positional[0].span.clone(), + Some(args.positional[0].span.clone()), )); } else { return Err(ShellError::string("Can not change to directory")); diff --git a/src/commands/classified.rs b/src/commands/classified.rs index a83a384f9..e77236327 100644 --- a/src/commands/classified.rs +++ b/src/commands/classified.rs @@ -151,38 +151,6 @@ impl InternalCommand { } } } - /* - let stream = result.filter_map(move |v| match v { - //ReturnValue::Value(Value::Error(err)) => futures::future::err(Err(err)), - ReturnValue::Action(action) => match action { - CommandAction::ChangePath(path) => { - env.lock().unwrap().last_mut().map(|x| { - x.path = path; - x - }); - futures::future::ready(None) - } - CommandAction::Enter(obj) => { - let new_env = Environment { - obj: obj, - path: PathBuf::from("/"), - }; - env.lock().unwrap().push(new_env); - futures::future::ready(None) - } - CommandAction::Exit => { - let mut v = env.lock().unwrap(); - if v.len() == 1 { - std::process::exit(0); - } - v.pop(); - futures::future::ready(None) - } - }, - - ReturnValue::Value(v) => futures::future::ready(Some(v)), - }); - */ Ok(stream.boxed() as InputStream) } } diff --git a/src/commands/enter.rs b/src/commands/enter.rs index 972225823..1b302b1b5 100644 --- a/src/commands/enter.rs +++ b/src/commands/enter.rs @@ -7,7 +7,11 @@ use std::path::{Path, PathBuf}; pub fn enter(args: CommandArgs) -> Result { if args.positional.len() == 0 { - return Err(ShellError::string("open requires a filepath or url")); + return Err(ShellError::maybe_labeled_error( + "open requires a path or url", + "missing path", + args.name_span, + )); } let cwd = args diff --git a/src/commands/first.rs b/src/commands/first.rs index afc1ff8f5..89bb4ff9c 100644 --- a/src/commands/first.rs +++ b/src/commands/first.rs @@ -5,15 +5,11 @@ use crate::prelude::*; pub fn first(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "First requires an amount", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("first requires an amount.")); - } + return Err(ShellError::maybe_labeled_error( + "First requires an amount", + "needs parameter", + args.name_span, + )); } let amount = args.positional[0].as_i64(); diff --git a/src/commands/from_xml.rs b/src/commands/from_xml.rs index 83e964e09..5d82d2657 100644 --- a/src/commands/from_xml.rs +++ b/src/commands/from_xml.rs @@ -15,7 +15,7 @@ fn from_node_to_value<'a, 'd>(n: &roxmltree::Node<'a, 'd>) -> Value { .filter(|x| match x { Value::Primitive(Primitive::String(f)) => { if f.trim() == "" { - false + false } else { true } @@ -57,11 +57,16 @@ pub fn from_xml_string_to_value(s: String) -> Value { pub fn from_xml(args: CommandArgs) -> Result { let out = args.input; + let span = args.name_span; Ok(out - .map(|a| match a { - Value::Primitive(Primitive::String(s)) => ReturnValue::Value(from_xml_string_to_value(s)), - _ => ReturnValue::Value(Value::Error(Box::new(ShellError::string( + .map(move |a| match a { + Value::Primitive(Primitive::String(s)) => { + ReturnValue::Value(from_xml_string_to_value(s)) + } + _ => ReturnValue::Value(Value::Error(Box::new(ShellError::maybe_labeled_error( "Trying to convert XML from non-string".to_string(), + "given non-string", + span, )))), }) .boxed()) diff --git a/src/commands/get.rs b/src/commands/get.rs index 3939e9fde..60b835d62 100644 --- a/src/commands/get.rs +++ b/src/commands/get.rs @@ -11,7 +11,7 @@ fn get_member(path: &str, span: Span, obj: &Value) -> Option { None => { return Some(Value::Error(Box::new(ShellError::labeled_error( "Unknown field", - "unknown field", + "object missing field", span, )))); } @@ -23,15 +23,11 @@ fn get_member(path: &str, span: Span, obj: &Value) -> Option { pub fn get(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "Get requires a field or field path", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("get requires fields.")); - } + return Err(ShellError::maybe_labeled_error( + "Get requires a field or field path", + "needs parameter", + args.name_span, + )); } let amount = args.positional[0].as_i64(); diff --git a/src/commands/ls.rs b/src/commands/ls.rs index bfa86218e..102cf7b9c 100644 --- a/src/commands/ls.rs +++ b/src/commands/ls.rs @@ -31,7 +31,11 @@ pub fn ls(args: CommandArgs) -> Result { s.span, )); } else { - return Err(ShellError::string(e.to_string())); + return Err(ShellError::maybe_labeled_error( + e.to_string(), + e.to_string(), + args.name_span, + )); } } Ok(o) => o, diff --git a/src/commands/open.rs b/src/commands/open.rs index 18b8e242d..b28067132 100644 --- a/src/commands/open.rs +++ b/src/commands/open.rs @@ -6,7 +6,11 @@ use std::path::{Path, PathBuf}; pub fn open(args: CommandArgs) -> Result { if args.positional.len() == 0 { - return Err(ShellError::string("open requires a filepath or url")); + return Err(ShellError::maybe_labeled_error( + "Open requires a path or url", + "needs path or url", + args.name_span, + )); } let cwd = args diff --git a/src/commands/pick.rs b/src/commands/pick.rs index 73e953ad0..9de5e8068 100644 --- a/src/commands/pick.rs +++ b/src/commands/pick.rs @@ -5,15 +5,11 @@ use crate::prelude::*; pub fn pick(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "Pick requires fields", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("pick requires fields.")); - } + return Err(ShellError::maybe_labeled_error( + "Pick requires fields", + "needs parameter", + args.name_span, + )); } let fields: Result, _> = args.positional.iter().map(|a| a.as_string()).collect(); diff --git a/src/commands/reject.rs b/src/commands/reject.rs index 372adcf7c..ec5703738 100644 --- a/src/commands/reject.rs +++ b/src/commands/reject.rs @@ -5,15 +5,11 @@ use crate::prelude::*; pub fn reject(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "Reject requires fields", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("reject requires fields.")); - } + return Err(ShellError::maybe_labeled_error( + "Reject requires fields", + "needs parameter", + args.name_span, + )); } let fields: Result, _> = args.positional.iter().map(|a| a.as_string()).collect(); diff --git a/src/commands/save.rs b/src/commands/save.rs index ea16f5f0b..b5e61e4a0 100644 --- a/src/commands/save.rs +++ b/src/commands/save.rs @@ -6,7 +6,11 @@ use std::path::{Path, PathBuf}; pub fn save(args: SinkCommandArgs) -> Result<(), ShellError> { if args.positional.len() == 0 { - return Err(ShellError::string("save requires a filepath")); + return Err(ShellError::maybe_labeled_error( + "Save requires a filepath", + "needs path", + args.name_span, + )); } let cwd = args diff --git a/src/commands/size.rs b/src/commands/size.rs index fb9ff767e..6fdf78cfa 100644 --- a/src/commands/size.rs +++ b/src/commands/size.rs @@ -6,8 +6,12 @@ use std::fs::File; use std::io::prelude::*; pub fn size(args: CommandArgs) -> Result { - if args.positional.is_empty() { - return Err(ShellError::string("size requires at least one file")); + if args.positional.len() == 0 { + return Err(ShellError::maybe_labeled_error( + "Size requires a filepath", + "needs path", + args.name_span, + )); } let cwd = args .env diff --git a/src/commands/skip.rs b/src/commands/skip.rs index 38af0c1a7..d55615105 100644 --- a/src/commands/skip.rs +++ b/src/commands/skip.rs @@ -3,15 +3,11 @@ use crate::prelude::*; pub fn skip(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "Skip requires an amount", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("skip requires an amount.")); - } + return Err(ShellError::maybe_labeled_error( + "Skip requires an amount", + "needs parameter", + args.name_span, + )); } let amount = args.positional[0].as_i64(); diff --git a/src/commands/view.rs b/src/commands/view.rs index 043b30f6a..51e29cdba 100644 --- a/src/commands/view.rs +++ b/src/commands/view.rs @@ -4,15 +4,11 @@ use prettyprint::PrettyPrinter; pub fn view(args: CommandArgs) -> Result { if args.positional.len() == 0 { - if let Some(span) = args.name_span { - return Err(ShellError::labeled_error( - "View requires a filename", - "needs parameter", - span, - )); - } else { - return Err(ShellError::string("view requires a filename.")); - } + return Err(ShellError::maybe_labeled_error( + "View requires a filename", + "needs parameter", + args.name_span, + )); } let target = match args.positional[0].as_string() { diff --git a/src/commands/where_.rs b/src/commands/where_.rs index 45ac0c7fe..045c6f7df 100644 --- a/src/commands/where_.rs +++ b/src/commands/where_.rs @@ -25,8 +25,12 @@ impl Command for Where { } pub fn r#where(args: CommandArgs) -> Result { - if args.positional.is_empty() { - return Err(ShellError::string("select requires a field")); + if args.positional.len() == 0 { + return Err(ShellError::maybe_labeled_error( + "Where requires a condition", + "needs condition", + args.name_span, + )); } let block = args.positional[0].as_block()?; diff --git a/src/errors.rs b/src/errors.rs index 1023474d5..62e7534db 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -53,6 +53,20 @@ impl ShellError { ) } + crate fn maybe_labeled_error( + msg: impl Into, + label: impl Into, + span: Option, + ) -> ShellError { + match span { + Some(span) => ShellError::diagnostic( + Diagnostic::new(Severity::Error, msg.into()) + .with_label(Label::new_primary(span).with_message(label.into())), + ), + None => ShellError::string(msg), + } + } + crate fn string(title: impl Into) -> ShellError { ShellError::String(StringError::new(title.into(), Value::nothing())) }