From 452d8c06e91cc3cc2ac918a55c2792b8a3333506 Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Sat, 2 Jan 2021 08:52:19 +1300 Subject: [PATCH] Improve some errors, streamline internal error handling (#2839) * Improve some errors, streamline internal error handling * Fix lints --- .../src/commands/classified/internal.rs | 59 ++++++++----------- crates/nu-cli/src/commands/enter.rs | 3 +- crates/nu-cli/src/commands/nu/plugin.rs | 2 +- crates/nu-cli/src/commands/open.rs | 18 ++++-- crates/nu-cli/src/evaluation_context.rs | 4 -- crates/nu-cli/src/examples.rs | 6 +- crates/nu-cli/tests/commands/enter.rs | 2 +- crates/nu-cli/tests/commands/open.rs | 2 +- 8 files changed, 46 insertions(+), 50 deletions(-) diff --git a/crates/nu-cli/src/commands/classified/internal.rs b/crates/nu-cli/src/commands/classified/internal.rs index 3a7ddeb8b9..d1a8b19cbf 100644 --- a/crates/nu-cli/src/commands/classified/internal.rs +++ b/crates/nu-cli/src/commands/classified/internal.rs @@ -53,12 +53,12 @@ pub(crate) async fn run_internal_command( Ok(ReturnSuccess::Action(action)) => match action { CommandAction::ChangePath(path) => { context.shell_manager.set_path(path); - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } CommandAction::Exit => std::process::exit(0), // TODO: save history.txt CommandAction::Error(err) => { - context.error(err.clone()); - InputStream::one(UntaggedValue::Error(err).into_untagged_value()) + context.error(err); + InputStream::empty() } CommandAction::AutoConvert(tagged_contents, extension) => { let contents_tag = tagged_contents.tag.clone(); @@ -117,8 +117,8 @@ pub(crate) async fn run_internal_command( futures::stream::iter(output).to_input_stream() } - Err(e) => { - context.add_error(e); + Err(err) => { + context.error(err); InputStream::empty() } } @@ -138,9 +138,8 @@ pub(crate) async fn run_internal_command( ) { Ok(v) => v, Err(err) => { - return InputStream::one( - UntaggedValue::Error(err).into_untagged_value(), - ) + context.error(err); + return InputStream::empty(); } }, )); @@ -151,9 +150,8 @@ pub(crate) async fn run_internal_command( match HelpShell::index(&context.scope) { Ok(v) => v, Err(err) => { - return InputStream::one( - UntaggedValue::Error(err).into_untagged_value(), - ) + context.error(err); + return InputStream::empty(); } }, )); @@ -171,10 +169,8 @@ pub(crate) async fn run_internal_command( match FilesystemShell::with_location(location) { Ok(v) => v, Err(err) => { - return InputStream::one( - UntaggedValue::Error(err.into()) - .into_untagged_value(), - ) + context.error(err.into()); + return InputStream::empty(); } }, )); @@ -198,12 +194,9 @@ pub(crate) async fn run_internal_command( .await; if let Err(err) = result { - return InputStream::one( - UntaggedValue::Error(err.into()) - .into_untagged_value(), - ); + context.error(err.into()); } - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } Err(_) => { context.error(ShellError::labeled_error( @@ -212,7 +205,7 @@ pub(crate) async fn run_internal_command( filename.span(), )); - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } } } @@ -228,39 +221,37 @@ pub(crate) async fn run_internal_command( .collect(), ); - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } Err(reason) => { - context.error(reason.clone()); - InputStream::one( - UntaggedValue::Error(reason).into_untagged_value(), - ) + context.error(reason); + InputStream::empty() } } } CommandAction::PreviousShell => { context.shell_manager.prev(); - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } CommandAction::NextShell => { context.shell_manager.next(); - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } CommandAction::LeaveShell => { context.shell_manager.remove_at_current(); if context.shell_manager.is_empty() { std::process::exit(0); // TODO: save history.txt } - InputStream::from_stream(futures::stream::iter(vec![])) + InputStream::empty() } }, Ok(ReturnSuccess::Value(Value { value: UntaggedValue::Error(err), - tag, + .. })) => { - context.error(err.clone()); - InputStream::one(UntaggedValue::Error(err).into_value(tag)) + context.error(err); + InputStream::empty() } Ok(ReturnSuccess::Value(v)) => InputStream::one(v), @@ -280,8 +271,8 @@ pub(crate) async fn run_internal_command( } Err(err) => { - context.error(err.clone()); - InputStream::one(UntaggedValue::Error(err).into_untagged_value()) + context.error(err); + InputStream::empty() } } } diff --git a/crates/nu-cli/src/commands/enter.rs b/crates/nu-cli/src/commands/enter.rs index e1d18ccdb9..a706127b09 100644 --- a/crates/nu-cli/src/commands/enter.rs +++ b/crates/nu-cli/src/commands/enter.rs @@ -112,11 +112,12 @@ async fn enter(raw_args: CommandArgs) -> Result { let cwd = shell_manager.path(); let full_path = std::path::PathBuf::from(cwd); + let span = location.span(); let (file_extension, tagged_contents) = crate::commands::open::fetch( &full_path, &PathBuf::from(location_clone), - tag.span, + span, encoding, ) .await?; diff --git a/crates/nu-cli/src/commands/nu/plugin.rs b/crates/nu-cli/src/commands/nu/plugin.rs index 3f0b90c226..7ab1a61263 100644 --- a/crates/nu-cli/src/commands/nu/plugin.rs +++ b/crates/nu-cli/src/commands/nu/plugin.rs @@ -42,7 +42,7 @@ impl WholeStreamCommand for SubCommand { vec![Example { description: "Load all plugins in the current directory", example: "nu plugin --load .", - result: Some(vec![]), + result: None, }] } diff --git a/crates/nu-cli/src/commands/open.rs b/crates/nu-cli/src/commands/open.rs index 20324b00b5..1d95126275 100644 --- a/crates/nu-cli/src/commands/open.rs +++ b/crates/nu-cli/src/commands/open.rs @@ -188,12 +188,20 @@ pub async fn fetch( // TODO: I don't understand the point of this? Maybe for better error reporting let mut cwd = cwd.clone(); cwd.push(location); - let nice_location = dunce::canonicalize(&cwd).map_err(|e| { - ShellError::labeled_error( - format!("Cannot canonicalize file {:?} because {:?}", &cwd, e), - "Cannot canonicalize", + let nice_location = dunce::canonicalize(&cwd).map_err(|e| match e.kind() { + std::io::ErrorKind::NotFound => ShellError::labeled_error( + format!("Cannot find file {:?}", cwd), + "cannot find file", span, - ) + ), + std::io::ErrorKind::PermissionDenied => { + ShellError::labeled_error("Permission denied", "permission denied", span) + } + _ => ShellError::labeled_error( + format!("Cannot open file {:?} because {:?}", &cwd, e), + "Cannot open", + span, + ), })?; // The extension may be used in AutoConvert later on diff --git a/crates/nu-cli/src/evaluation_context.rs b/crates/nu-cli/src/evaluation_context.rs index eea740d44a..8dfe3328f4 100644 --- a/crates/nu-cli/src/evaluation_context.rs +++ b/crates/nu-cli/src/evaluation_context.rs @@ -74,10 +74,6 @@ impl EvaluationContext { self.current_errors.lock().clone() } - pub(crate) fn add_error(&self, err: ShellError) { - self.current_errors.lock().push(err); - } - pub(crate) fn maybe_print_errors(&self, source: Text) -> bool { let errors = self.current_errors.clone(); let mut errors = errors.lock(); diff --git a/crates/nu-cli/src/examples.rs b/crates/nu-cli/src/examples.rs index 1f7bba77f1..bf0f72b475 100644 --- a/crates/nu-cli/src/examples.rs +++ b/crates/nu-cli/src/examples.rs @@ -49,7 +49,7 @@ pub fn test_examples(cmd: Command) -> Result<(), ShellError> { for sample_pipeline in examples { let mut ctx = base_context.clone(); - let block = parse_line(sample_pipeline.example, &mut ctx)?; + let block = parse_line(sample_pipeline.example, &ctx)?; println!("{:#?}", block); @@ -107,7 +107,7 @@ pub fn test(cmd: impl WholeStreamCommand + 'static) -> Result<(), ShellError> { for sample_pipeline in examples { let mut ctx = base_context.clone(); - let block = parse_line(sample_pipeline.example, &mut ctx)?; + let block = parse_line(sample_pipeline.example, &ctx)?; if let Some(expected) = &sample_pipeline.result { let result = block_on(evaluate_block(block, &mut ctx))?; @@ -171,7 +171,7 @@ pub fn test_anchors(cmd: Command) -> Result<(), ShellError> { let mut ctx = base_context.clone(); - let block = parse_line(&pipeline_with_anchor, &mut ctx)?; + let block = parse_line(&pipeline_with_anchor, &ctx)?; let result = block_on(evaluate_block(block, &mut ctx))?; ctx.with_errors(|reasons| reasons.iter().cloned().take(1).next()) diff --git a/crates/nu-cli/tests/commands/enter.rs b/crates/nu-cli/tests/commands/enter.rs index 3156f75415..d7377eade6 100644 --- a/crates/nu-cli/tests/commands/enter.rs +++ b/crates/nu-cli/tests/commands/enter.rs @@ -80,6 +80,6 @@ fn errors_if_file_not_found() { "enter i_dont_exist.csv" ); - assert!(actual.err.contains("Cannot canonicalize")); + assert!(actual.err.contains("Cannot find file")); }) } diff --git a/crates/nu-cli/tests/commands/open.rs b/crates/nu-cli/tests/commands/open.rs index 9293a9165d..4aec9f2a25 100644 --- a/crates/nu-cli/tests/commands/open.rs +++ b/crates/nu-cli/tests/commands/open.rs @@ -222,7 +222,7 @@ fn errors_if_file_not_found() { cwd: "tests/fixtures/formats", "open i_dont_exist.txt" ); - let expected = "Cannot canonicalize"; + let expected = "Cannot find file"; assert!( actual.err.contains(expected), "Error:\n{}\ndoes not contain{}",