From 903afda6d946960682dc5234724a09d2bfed288b Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Thu, 15 Feb 2024 17:25:50 +0000 Subject: [PATCH] Remove required positional arg for some file system commands (#11858) # Description Fixes (most of) #11796. Some filesystem commands have a required positional argument which hinders spreading rest args. This PR removes the required positional arg from `rm`, `open`, and `touch` to be consistent with other filesystem commands that already only have a single rest arg (`mkdir` and `cp`). # User-Facing Changes `rm`, `open`, and `touch` might no longer error when they used to, but otherwise there should be no noticeable changes. --- crates/nu-command/src/filesystem/open.rs | 38 ++++++----------------- crates/nu-command/src/filesystem/rm.rs | 38 +++++++++++------------ crates/nu-command/src/filesystem/touch.rs | 21 +++++++------ 3 files changed, 39 insertions(+), 58 deletions(-) diff --git a/crates/nu-command/src/filesystem/open.rs b/crates/nu-command/src/filesystem/open.rs index 7611212a5e..21989e8e3d 100644 --- a/crates/nu-command/src/filesystem/open.rs +++ b/crates/nu-command/src/filesystem/open.rs @@ -5,7 +5,7 @@ use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::util::BufferedReader; use nu_protocol::{ Category, DataSource, Example, IntoInterruptiblePipelineData, NuPath, PipelineData, - PipelineMetadata, RawStream, ShellError, Signature, Spanned, SyntaxShape, Type, Value, + PipelineMetadata, RawStream, ShellError, Signature, Spanned, SyntaxShape, Type, }; use std::io::BufReader; @@ -42,12 +42,7 @@ impl Command for Open { fn signature(&self) -> nu_protocol::Signature { Signature::build("open") .input_output_types(vec![(Type::Nothing, Type::Any), (Type::String, Type::Any)]) - .optional("filename", SyntaxShape::GlobPattern, "The filename to use.") - .rest( - "filenames", - SyntaxShape::GlobPattern, - "Optional additional files to open.", - ) + .rest("files", SyntaxShape::GlobPattern, "The file(s) to open.") .switch("raw", "open file as raw binary", Some('r')) .category(Category::FileSystem) } @@ -63,21 +58,11 @@ impl Command for Open { let call_span = call.head; let ctrlc = engine_state.ctrlc.clone(); let cwd = current_dir(engine_state, stack)?; - let req_path = call.opt::>(engine_state, stack, 0)?; - let mut path_params = call.rest::>(engine_state, stack, 1)?; + let mut paths = call.rest::>(engine_state, stack, 0)?; - // FIXME: JT: what is this doing here? - - if let Some(filename) = req_path { - path_params.insert(0, filename); - } else { + if paths.is_empty() && call.rest_iter(0).next().is_none() { + // try to use path from pipeline input if there were no positional or spread args let filename = match input { - PipelineData::Value(Value::Nothing { .. }, ..) => { - return Err(ShellError::MissingParameter { - param_name: "needs filename".to_string(), - span: call.head, - }) - } PipelineData::Value(val, ..) => val.as_spanned_string()?, _ => { return Err(ShellError::MissingParameter { @@ -87,18 +72,15 @@ impl Command for Open { } }; - path_params.insert( - 0, - Spanned { - item: NuPath::UnQuoted(filename.item), - span: filename.span, - }, - ); + paths.push(Spanned { + item: NuPath::UnQuoted(filename.item), + span: filename.span, + }); } let mut output = vec![]; - for mut path in path_params.into_iter() { + for mut path in paths { //FIXME: `open` should not have to do this path.item = path.item.strip_ansi_string_unlikely(); diff --git a/crates/nu-command/src/filesystem/rm.rs b/crates/nu-command/src/filesystem/rm.rs index b601ac988c..abf4cb19f6 100644 --- a/crates/nu-command/src/filesystem/rm.rs +++ b/crates/nu-command/src/filesystem/rm.rs @@ -40,13 +40,9 @@ impl Command for Rm { } fn signature(&self) -> Signature { - let sig = Signature::build("rm") + Signature::build("rm") .input_output_types(vec![(Type::Nothing, Type::Nothing)]) - .required( - "filename", - SyntaxShape::GlobPattern, - "The file or files you want to remove.", - ) + .rest("paths", SyntaxShape::GlobPattern, "The file paths(s) to remove.") .switch( "trash", "move to the platform's trash instead of permanently deleting. not used on android and ios", @@ -56,8 +52,8 @@ impl Command for Rm { "permanent", "delete permanently, ignoring the 'always_trash' config option. always enabled on android and ios", Some('p'), - ); - sig.switch("recursive", "delete subdirectories recursively", Some('r')) + ) + .switch("recursive", "delete subdirectories recursively", Some('r')) .switch("force", "suppress error when no file", Some('f')) .switch("verbose", "print names of deleted files", Some('v')) .switch("interactive", "ask user to confirm action", Some('i')) @@ -66,11 +62,6 @@ impl Command for Rm { "ask user to confirm action only once", Some('I'), ) - .rest( - "rest", - SyntaxShape::GlobPattern, - "Additional file path(s) to remove.", - ) .category(Category::FileSystem) } @@ -135,7 +126,14 @@ fn rm( let ctrlc = engine_state.ctrlc.clone(); - let mut targets: Vec> = call.rest(engine_state, stack, 0)?; + let mut paths: Vec> = call.rest(engine_state, stack, 0)?; + + if paths.is_empty() { + return Err(ShellError::MissingParameter { + param_name: "requires file paths".to_string(), + span: call.head, + }); + } let mut unique_argument_check = None; @@ -156,7 +154,7 @@ fn rm( .into() }); - for (idx, path) in targets.clone().into_iter().enumerate() { + for (idx, path) in paths.clone().into_iter().enumerate() { if let Some(ref home) = home { if expand_path_with(path.item.as_ref(), ¤tdir_path) .to_string_lossy() @@ -173,7 +171,7 @@ fn rm( }, span: path.span, }; - let _ = std::mem::replace(&mut targets[idx], corrected_path); + let _ = std::mem::replace(&mut paths[idx], corrected_path); } let span = call.head; @@ -204,7 +202,7 @@ fn rm( } } - if targets.is_empty() { + if paths.is_empty() { return Err(ShellError::GenericError { error: "rm requires target paths".into(), msg: "needs parameter".into(), @@ -225,12 +223,12 @@ fn rm( } let targets_span = Span::new( - targets + paths .iter() .map(|x| x.span.start) .min() .expect("targets were empty"), - targets + paths .iter() .map(|x| x.span.end) .max() @@ -240,7 +238,7 @@ fn rm( let (mut target_exists, mut empty_span) = (false, call.head); let mut all_targets: HashMap = HashMap::new(); - for target in targets { + for target in paths { let path = expand_path_with(target.item.as_ref(), ¤tdir_path); if currentdir_path.to_string_lossy() == path.to_string_lossy() || currentdir_path.starts_with(format!("{}{}", target.item, std::path::MAIN_SEPARATOR)) diff --git a/crates/nu-command/src/filesystem/touch.rs b/crates/nu-command/src/filesystem/touch.rs index 4a13f66c03..15c7da865c 100644 --- a/crates/nu-command/src/filesystem/touch.rs +++ b/crates/nu-command/src/filesystem/touch.rs @@ -25,12 +25,8 @@ impl Command for Touch { fn signature(&self) -> Signature { Signature::build("touch") - .input_output_types(vec![ (Type::Nothing, Type::Nothing) ]) - .required( - "filename", - SyntaxShape::Filepath, - "The path of the file you want to create.", - ) + .input_output_types(vec![(Type::Nothing, Type::Nothing)]) + .rest("files", SyntaxShape::Filepath, "The file(s) to create.") .named( "reference", SyntaxShape::String, @@ -52,7 +48,6 @@ impl Command for Touch { "do not create the file if it does not exist", Some('c'), ) - .rest("rest", SyntaxShape::Filepath, "Additional files to create.") .category(Category::FileSystem) } @@ -71,8 +66,14 @@ impl Command for Touch { let mut change_atime: bool = call.has_flag(engine_state, stack, "access")?; let use_reference: bool = call.has_flag(engine_state, stack, "reference")?; let no_create: bool = call.has_flag(engine_state, stack, "no-create")?; - let target: String = call.req(engine_state, stack, 0)?; - let rest: Vec = call.rest(engine_state, stack, 1)?; + let files: Vec = call.rest(engine_state, stack, 0)?; + + if files.is_empty() { + return Err(ShellError::MissingParameter { + param_name: "requires file paths".to_string(), + span: call.head, + }); + } let mut date: Option> = None; let mut ref_date_atime: Option> = None; @@ -127,7 +128,7 @@ impl Command for Touch { } } - for (index, item) in vec![target].into_iter().chain(rest).enumerate() { + for (index, item) in files.into_iter().enumerate() { if no_create { let path = Path::new(&item); if !path.exists() {