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.
nushell · Feb 15, 2024 · 903afda · 903afda
1 parent 317653d
commit 903afda
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 58 deletions.
diff --git 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::<Spanned<NuPath>>(engine_state, stack, 0)?;
-        let mut path_params = call.rest::<Spanned<NuPath>>(engine_state, stack, 1)?;
+        let mut paths = call.rest::<Spanned<NuPath>>(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
@@ -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<Spanned<NuPath>> = call.rest(engine_state, stack, 0)?;
+    let mut paths: Vec<Spanned<NuPath>> = 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(), &currentdir_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<PathBuf, Span> = HashMap::new();
 
-    for target in targets {
+    for target in paths {
         let path = expand_path_with(target.item.as_ref(), &currentdir_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
@@ -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<String> = call.rest(engine_state, stack, 1)?;
+        let files: Vec<String> = 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<DateTime<Local>> = None;
         let mut ref_date_atime: Option<DateTime<Local>> = 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() {