remove .unwrap() and .expect() in shell code (#663)

* remove .unwrap() and .expect() in shell code this should provide nicer error messages if users get to these situations * Silent github comments
Schniz · Feb 10, 2022 · c85ff97 · c85ff97
1 parent 5d06957
commit c85ff97
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 45 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -30,6 +30,7 @@ url = "2.2.2"
 sysinfo = "0.23.0"
 thiserror = "1.0.30"
 clap_complete = "3.0.6"
+anyhow = "1.0.53"
 
 [dev-dependencies]
 pretty_assertions = "1.1.0"

diff --git a/site/vercel.json b/site/vercel.json
@@ -1,17 +1,19 @@
 {
+  "$schema": "https://openapi.vercel.sh/vercel.json",
+  "github": {
+    "silent": true
+  },
   "redirects": [
     { "source": "/", "destination": "https://github.com/Schniz/fnm" }
   ],
-  "rewrites": [
-    { "source": "/install", "destination": "/install.txt" }
-  ],
+  "rewrites": [{ "source": "/install", "destination": "/install.txt" }],
   "headers": [
     {
       "source": "/install",
-      "headers" : [
+      "headers": [
         {
-          "key" : "Cache-Control",
-          "value" : "public, max-age=3600"
+          "key": "Cache-Control",
+          "value": "public, max-age=3600"
         }
       ]
     }

diff --git a/src/commands/env.rs b/src/commands/env.rs
@@ -69,7 +69,7 @@ impl Command for Env {
         } else {
             multishell_path.join("bin")
         };
-        println!("{}", shell.path(&binary_path));
+        println!("{}", shell.path(&binary_path)?);
         println!(
             "{}",
             shell.set_env_var("FNM_MULTISHELL_PATH", multishell_path.to_str().unwrap())
@@ -98,7 +98,7 @@ impl Command for Env {
             shell.set_env_var("FNM_ARCH", &config.arch.to_string())
         );
         if self.use_on_cd {
-            println!("{}", shell.use_on_cd(config));
+            println!("{}", shell.use_on_cd(config)?);
         }
         if let Some(v) = shell.rehash() {
             println!("{}", v);
@@ -123,6 +123,11 @@ pub enum Error {
         source: std::io::Error,
         temp_dir: std::path::PathBuf,
     },
+    #[error(transparent)]
+    ShellError {
+        #[from]
+        source: anyhow::Error,
+    },
 }
 
 fn shells_as_string() -> String {

diff --git a/src/shell/bash.rs b/src/shell/bash.rs
@@ -12,15 +12,18 @@ impl Shell for Bash {
         clap_complete::Shell::Bash
     }
 
-    fn path(&self, path: &Path) -> String {
-        format!("export PATH={:?}:$PATH", path.to_str().unwrap())
+    fn path(&self, path: &Path) -> anyhow::Result<String> {
+        let path = path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Can't convert path to string"))?;
+        Ok(format!("export PATH={:?}:$PATH", path))
     }
 
     fn set_env_var(&self, name: &str, value: &str) -> String {
         format!("export {}={:?}", name, value)
     }
 
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String {
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String> {
         let autoload_hook = match config.version_file_strategy() {
             VersionFileStrategy::Local => indoc!(
                 r#"
@@ -31,7 +34,7 @@ impl Shell for Bash {
             ),
             VersionFileStrategy::Recursive => r#"fnm use --silent-if-unchanged"#,
         };
-        formatdoc!(
+        Ok(formatdoc!(
             r#"
                 __fnm_use_if_file_found() {{
                     {autoload_hook}
@@ -46,6 +49,6 @@ impl Shell for Bash {
                 __fnm_use_if_file_found
             "#,
             autoload_hook = autoload_hook
-        )
+        ))
     }
 }
diff --git a/src/shell/fish.rs b/src/shell/fish.rs
@@ -12,15 +12,18 @@ impl Shell for Fish {
         clap_complete::Shell::Fish
     }
 
-    fn path(&self, path: &Path) -> String {
-        format!("set -gx PATH {:?} $PATH;", path.to_str().unwrap())
+    fn path(&self, path: &Path) -> anyhow::Result<String> {
+        let path = path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Can't convert path to string"))?;
+        Ok(format!("set -gx PATH {:?} $PATH;", path))
     }
 
     fn set_env_var(&self, name: &str, value: &str) -> String {
         format!("set -gx {name} {value:?};", name = name, value = value)
     }
 
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String {
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String> {
         let autoload_hook = match config.version_file_strategy() {
             VersionFileStrategy::Local => indoc!(
                 r#"
@@ -31,7 +34,7 @@ impl Shell for Fish {
             ),
             VersionFileStrategy::Recursive => r#"fnm use --silent-if-unchanged"#,
         };
-        formatdoc!(
+        Ok(formatdoc!(
             r#"
                 function _fnm_autoload_hook --on-variable PWD --description 'Change Node version on directory change'
                     status --is-command-substitution; and return
@@ -41,6 +44,6 @@ impl Shell for Fish {
                 _fnm_autoload_hook
             "#,
             autoload_hook = autoload_hook
-        )
+        ))
     }
 }
diff --git a/src/shell/infer/unix.rs b/src/shell/infer/unix.rs
@@ -107,15 +107,15 @@ mod tests {
     use std::process::{Command, Stdio};
 
     #[test]
-    fn test_get_process_info() {
+    fn test_get_process_info() -> anyhow::Result<()> {
         let subprocess = Command::new("bash")
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
-            .spawn()
-            .expect("Can't execute command");
+            .spawn()?;
         let process_info = get_process_info(subprocess.id());
         let parent_pid = process_info.ok().and_then(|x| x.parent_pid);
         assert_eq!(parent_pid, Some(std::process::id()));
+        Ok(())
     }
 }
diff --git a/src/shell/powershell.rs b/src/shell/powershell.rs
@@ -8,19 +8,24 @@ use std::path::Path;
 pub struct PowerShell;
 
 impl Shell for PowerShell {
-    fn path(&self, path: &Path) -> String {
-        let current_path = std::env::var_os("PATH").expect("Can't read PATH env var");
+    fn path(&self, path: &Path) -> anyhow::Result<String> {
+        let current_path =
+            std::env::var_os("PATH").ok_or_else(|| anyhow::anyhow!("Can't read PATH env var"))?;
         let mut split_paths: Vec<_> = std::env::split_paths(&current_path).collect();
         split_paths.insert(0, path.to_path_buf());
-        let new_path = std::env::join_paths(split_paths).expect("Can't join paths");
-        self.set_env_var("PATH", new_path.to_str().expect("Can't read PATH"))
+        let new_path = std::env::join_paths(split_paths)
+            .map_err(|source| anyhow::anyhow!("Can't join paths: {}", source))?;
+        let new_path = new_path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Can't read PATH"))?;
+        Ok(self.set_env_var("PATH", new_path))
     }
 
     fn set_env_var(&self, name: &str, value: &str) -> String {
         format!(r#"$env:{} = "{}""#, name, value)
     }
 
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String {
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String> {
         let autoload_hook = match config.version_file_strategy() {
             VersionFileStrategy::Local => indoc!(
                 r#"
@@ -29,7 +34,7 @@ impl Shell for PowerShell {
             ),
             VersionFileStrategy::Recursive => r#"fnm use --silent-if-unchanged"#,
         };
-        formatdoc!(
+        Ok(formatdoc!(
             r#"
                 function Set-FnmOnLoad {{ {autoload_hook} }}
                 function Set-LocationWithFnm {{ param($path); Set-Location $path; Set-FnmOnLoad }}
@@ -39,7 +44,7 @@ impl Shell for PowerShell {
                 Set-FnmOnLoad
             "#,
             autoload_hook = autoload_hook
-        )
+        ))
     }
     fn to_clap_shell(&self) -> clap_complete::Shell {
         clap_complete::Shell::PowerShell

diff --git a/src/shell/shell.rs b/src/shell/shell.rs
@@ -2,9 +2,9 @@ use std::fmt::Debug;
 use std::path::Path;
 
 pub trait Shell: Debug {
-    fn path(&self, path: &Path) -> String;
+    fn path(&self, path: &Path) -> anyhow::Result<String>;
     fn set_env_var(&self, name: &str, value: &str) -> String;
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String;
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String>;
     fn rehash(&self) -> Option<String> {
         None
     }

diff --git a/src/shell/windows_cmd/mod.rs b/src/shell/windows_cmd/mod.rs
@@ -10,25 +10,36 @@ impl Shell for WindowsCmd {
         panic!("Shell completion is not supported for Windows Command Prompt. Maybe try using PowerShell for a better experience?");
     }
 
-    fn path(&self, path: &Path) -> String {
-        let current_path = std::env::var_os("path").expect("Can't read PATH env var");
+    fn path(&self, path: &Path) -> anyhow::Result<String> {
+        let current_path =
+            std::env::var_os("path").ok_or_else(|| anyhow::anyhow!("Can't read PATH env var"))?;
         let mut split_paths: Vec<_> = std::env::split_paths(&current_path).collect();
         split_paths.insert(0, path.to_path_buf());
-        let new_path = std::env::join_paths(split_paths).expect("Can't join paths");
-        format!("SET PATH={}", new_path.to_str().expect("Can't read PATH"))
+        let new_path = std::env::join_paths(split_paths)
+            .map_err(|err| anyhow::anyhow!("Can't join paths: {}", err))?;
+        let new_path = new_path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Can't convert path to string"))?;
+        Ok(format!("SET PATH={}", new_path))
     }
 
     fn set_env_var(&self, name: &str, value: &str) -> String {
         format!("SET {}={}", name, value)
     }
 
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String {
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String> {
         let path = config.base_dir_with_default().join("cd.cmd");
-        create_cd_file_at(&path).expect("Can't create cd.cmd file for use-on-cd");
-        format!(
-            "doskey cd={} $*",
-            path.to_str().expect("Can't read path to cd.cmd")
-        )
+        create_cd_file_at(&path).map_err(|source| {
+            anyhow::anyhow!(
+                "Can't create cd.cmd file for use-on-cd at {}: {}",
+                path.display(),
+                source
+            )
+        })?;
+        let path = path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Can't read path to cd.cmd"))?;
+        Ok(format!("doskey cd={} $*", path,))
     }
 }
 

diff --git a/src/shell/zsh.rs b/src/shell/zsh.rs
@@ -12,8 +12,11 @@ impl Shell for Zsh {
         clap_complete::Shell::Zsh
     }
 
-    fn path(&self, path: &Path) -> String {
-        format!("export PATH={:?}:$PATH", path.to_str().unwrap())
+    fn path(&self, path: &Path) -> anyhow::Result<String> {
+        let path = path
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Path is not valid UTF-8"))?;
+        Ok(format!("export PATH={:?}:$PATH", path))
     }
 
     fn set_env_var(&self, name: &str, value: &str) -> String {
@@ -24,7 +27,7 @@ impl Shell for Zsh {
         Some("rehash".to_string())
     }
 
-    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> String {
+    fn use_on_cd(&self, config: &crate::config::FnmConfig) -> anyhow::Result<String> {
         let autoload_hook = match config.version_file_strategy() {
             VersionFileStrategy::Local => indoc!(
                 r#"
@@ -35,7 +38,7 @@ impl Shell for Zsh {
             ),
             VersionFileStrategy::Recursive => r#"fnm use --silent-if-unchanged"#,
         };
-        formatdoc!(
+        Ok(formatdoc!(
             r#"
                 autoload -U add-zsh-hook
                 _fnm_autoload_hook () {{
@@ -46,6 +49,6 @@ impl Shell for Zsh {
                     && _fnm_autoload_hook
             "#,
             autoload_hook = autoload_hook
-        )
+        ))
     }
 }