feat: a collection of small improvements (#70)

crates/cli/src/commands/add.rs

@@ -153,7 +153,7 @@ mod tests {
         };
         manager.add(&args).await.unwrap();
 
-        insta::assert_debug_snapshot!(get_all_folders(&dir.path().to_path_buf()));
+        insta::assert_debug_snapshot!(get_all_folders(dir.path()));
 
         // Ensure that is-buffer does not have any dependencies
         let is_buffer_path = virtual_store_dir.join("[email protected]/node_modules");
@@ -190,7 +190,7 @@ mod tests {
         };
         manager.add(&args).await.unwrap();
 
-        insta::assert_debug_snapshot!(get_all_folders(&dir.path().to_path_buf()));
+        insta::assert_debug_snapshot!(get_all_folders(dir.path()));
 
         // Make sure the symlinks are correct
         assert_eq!(
@@ -219,7 +219,7 @@ mod tests {
             virtual_store_dir: virtual_store_dir.to_string_lossy().to_string(),
         };
         manager.add(&args).await.unwrap();
-        let file = PackageJson::from_path(&dir.path().join("package.json")).unwrap();
+        let file = PackageJson::from_path(dir.path().join("package.json")).unwrap();
         assert!(file.get_dependencies(&[DependencyGroup::Default]).contains_key("is-odd"));
         env::set_current_dir(&current_directory).unwrap();
     }

crates/cli/src/commands/install.rs

@@ -99,7 +99,7 @@ mod tests {
         env::set_current_dir(dir.path()).unwrap();
 
         let package_json_path = dir.path().join("package.json");
-        let mut package_json = PackageJson::create_if_needed(&package_json_path).unwrap();
+        let mut package_json = PackageJson::create_if_needed(package_json_path.clone()).unwrap();
 
         package_json.add_dependency("is-odd", "3.0.1", DependencyGroup::Default).unwrap();
         package_json
@@ -122,7 +122,7 @@ mod tests {
         assert!(dir.path().join("node_modules/fast-decode-uri-component").is_symlink());
         assert!(dir.path().join("node_modules/.pacquet/[email protected]").is_dir());
 
-        insta::assert_debug_snapshot!(get_all_folders(&dir.path().to_path_buf()));
+        insta::assert_debug_snapshot!(get_all_folders(dir.path()));
 
         env::set_current_dir(&current_directory).unwrap();
     }

crates/cli/src/fs.rs

@@ -22,19 +22,19 @@ pub fn get_filenames_in_folder(path: &Path) -> Vec<String> {
 }
 
 #[cfg(test)]
-pub fn get_all_folders(root: &std::path::PathBuf) -> Vec<String> {
+pub fn get_all_folders(root: &std::path::Path) -> Vec<String> {
     let mut files = Vec::new();
     for entry in walkdir::WalkDir::new(root) {
         let entry = entry.unwrap();
-        let entry_path = entry.path().to_path_buf();
+        let entry_path = entry.path();
         if entry.file_type().is_dir() || entry.file_type().is_symlink() {
             // We need this mutation to ensure that both Unix and Windows paths resolves the same.
             // TODO: Find a better way to do this?
             let simple_path = entry_path
                 .strip_prefix(root)
                 .unwrap()
                 .components()
-                .map(|c| c.as_os_str().to_string_lossy().to_string())
+                .map(|c| c.as_os_str().to_str().expect("invalid UTF-8"))
                 .collect::<Vec<_>>()
                 .join("/");
 

crates/cli/src/lib.rs

@@ -48,14 +48,14 @@ async fn run_commands(cli: Cli) -> Result<()> {
                 .wrap_err("installing dependencies")?;
         }
         Subcommands::Test => {
-            let package_json = PackageJson::from_path(&package_json_path)
+            let package_json = PackageJson::from_path(package_json_path)
                 .wrap_err("getting the package.json in current directory")?;
             if let Some(script) = package_json.get_script("test", false)? {
                 execute_shell(script).wrap_err(format!("executing command: \"{0}\"", script))?;
             }
         }
         Subcommands::Run(args) => {
-            let package_json = PackageJson::from_path(&package_json_path)
+            let package_json = PackageJson::from_path(package_json_path)
                 .wrap_err("getting the package.json in current directory")?;
             if let Some(script) = package_json.get_script(&args.command, args.if_present)? {
                 let mut command = script.to_string();
@@ -71,7 +71,7 @@ async fn run_commands(cli: Cli) -> Result<()> {
             // object. If no start property is specified on the scripts object, it will attempt to
             // run node server.js as a default, failing if neither are present.
             // The intended usage of the property is to specify a command that starts your program.
-            let package_json = PackageJson::from_path(&package_json_path)
+            let package_json = PackageJson::from_path(package_json_path)
                 .wrap_err("getting the package.json in current directory")?;
             let command = if let Some(script) = package_json.get_script("start", true)? {
                 script

crates/cli/src/package_manager.rs

@@ -37,7 +37,7 @@ impl PackageManager {
     pub fn new<P: Into<PathBuf>>(package_json_path: P) -> Result<Self, PackageManagerError> {
         Ok(PackageManager {
             config: Box::new(get_current_npmrc()),
-            package_json: Box::new(PackageJson::create_if_needed(&package_json_path.into())?),
+            package_json: Box::new(PackageJson::create_if_needed(package_json_path.into())?),
             http_client: Box::new(reqwest::Client::new()),
         })
     }

crates/npmrc/src/custom_deserializer.rs

@@ -1,8 +1,4 @@
-use std::{
-    env,
-    path::{Path, PathBuf},
-    str::FromStr,
-};
+use std::{env, path::PathBuf, str::FromStr};
 
 use serde::{de, Deserialize, Deserializer};
 
@@ -28,11 +24,11 @@ pub fn default_public_hoist_pattern() -> Vec<String> {
 pub fn default_store_dir() -> PathBuf {
     // TODO: If env variables start with ~, make sure to resolve it into home_dir.
     if let Ok(pacquet_home) = env::var("PACQUET_HOME") {
-        return Path::new(&pacquet_home).join("store");
+        return PathBuf::from(pacquet_home).join("store");
     }
 
     if let Ok(xdg_data_home) = env::var("XDG_DATA_HOME") {
-        return Path::new(&xdg_data_home).join("pacquet/store");
+        return PathBuf::from(xdg_data_home).join("pacquet/store");
     }
 
     // Using ~ (tilde) for defining home path is not supported in Rust and
@@ -44,7 +40,7 @@ pub fn default_store_dir() -> PathBuf {
         "linux" => home_dir.join(".local/share/pacquet/store"),
         "macos" => home_dir.join("Library/pacquet/store"),
         "windows" => home_dir.join("AppData/Local/pacquet/store"),
-        _ => panic!("unsupported operating system: {0}", env::consts::OS),
+        _ => panic!("unsupported operating system: {}", env::consts::OS),
     }
 }
 
@@ -110,7 +106,7 @@ where
 
 #[cfg(test)]
 mod tests {
-    use std::env;
+    use std::{env, path::Path};
 
     use super::*;
     #[test]

crates/npmrc/src/lib.rs

@@ -169,7 +169,7 @@ pub struct Npmrc {
 
 impl Npmrc {
     pub fn new() -> Self {
-        let config: Npmrc = serde_ini::from_str("").unwrap();
+        let config: Npmrc = serde_ini::from_str("").unwrap(); // TODO: derive `SmartDefault` for `Npmrc and call `Npmrc::default()`
         config
     }
 }

crates/package_json/src/lib.rs

@@ -1,4 +1,9 @@
-use std::{collections::HashMap, convert::Into, fs, io::Write, path::PathBuf};
+use std::{
+    collections::HashMap,
+    fs,
+    io::Write,
+    path::{Path, PathBuf},
+};
 
 use pacquet_diagnostics::{
     miette::{self, Diagnostic},
@@ -58,12 +63,8 @@ pub struct PackageJson {
 }
 
 impl PackageJson {
-    pub fn new(path: PathBuf, value: Value) -> PackageJson {
-        PackageJson { path, value }
-    }
-
-    fn get_init_package_json(name: &str) -> Result<Value, PackageJsonError> {
-        let package_json = json!({
+    fn get_init_package_json(name: &str) -> Value {
+        json!({
             "name": name,
             "version": "1.0.0",
             "description": "",
@@ -74,66 +75,57 @@ impl PackageJson {
             "keywords": [],
             "author": "",
             "license": "ISC"
-        });
-        Ok(package_json)
-    }
-
-    fn to_string_prettify(package_json: &Value) -> Result<String, PackageJsonError> {
-        Ok(serde_json::to_string_pretty(package_json)?)
+        })
     }
 
-    fn write_to_file(path: &PathBuf) -> Result<Value, PackageJsonError> {
-        let mut file = fs::File::create(path)?;
+    fn write_to_file(path: &Path) -> Result<(Value, String), PackageJsonError> {
         let name = path
             .parent()
             .and_then(|folder| folder.file_name())
             .and_then(|file_name| file_name.to_str())
             .unwrap_or("");
-        let package_json = PackageJson::get_init_package_json(name)?;
-        let contents = PackageJson::to_string_prettify(&package_json)?;
-        file.write_all(contents.as_bytes())?;
-        Ok(package_json)
+        let package_json = PackageJson::get_init_package_json(name);
+        let contents = serde_json::to_string_pretty(&package_json)?;
+        fs::write(path, &contents)?; // TODO: forbid overwriting existing files
+        Ok((package_json, contents))
     }
 
-    fn read_from_file(path: &PathBuf) -> Result<Value, PackageJsonError> {
-        let file = fs::File::open(path)?;
-        Ok(serde_json::from_reader(file)?)
+    fn read_from_file(path: &Path) -> Result<Value, PackageJsonError> {
+        let contents = fs::read_to_string(path)?;
+        serde_json::from_str(&contents).map_err(PackageJsonError::from)
     }
 
-    pub fn init(path: &PathBuf) -> Result<(), PackageJsonError> {
+    pub fn init(path: &Path) -> Result<(), PackageJsonError> {
         if path.exists() {
             return Err(PackageJsonError::AlreadyExist);
         }
-        let package_json = PackageJson::write_to_file(path)?;
-        println!(
-            "Wrote to {}\n\n{}",
-            path.display(),
-            PackageJson::to_string_prettify(&package_json).unwrap_or(String::new())
-        );
+        let (_, contents) = PackageJson::write_to_file(path)?;
+        println!("Wrote to {path}\n\n{contents}", path = path.display());
         Ok(())
     }
 
-    pub fn from_path(path: &PathBuf) -> Result<PackageJson, PackageJsonError> {
+    pub fn from_path(path: PathBuf) -> Result<PackageJson, PackageJsonError> {
         if !path.exists() {
             return Err(PackageJsonError::NoImporterManifestFound(path.display().to_string()));
         }
 
-        Ok(PackageJson { path: path.to_path_buf(), value: PackageJson::read_from_file(path)? })
+        let value = PackageJson::read_from_file(&path)?;
+        Ok(PackageJson { path, value })
     }
 
-    pub fn create_if_needed(path: &PathBuf) -> Result<PackageJson, PackageJsonError> {
+    pub fn create_if_needed(path: PathBuf) -> Result<PackageJson, PackageJsonError> {
         let value = if path.exists() {
-            PackageJson::read_from_file(path)?
+            PackageJson::read_from_file(&path)?
         } else {
-            PackageJson::write_to_file(path)?
+            PackageJson::write_to_file(&path).map(|(value, _)| value)?
         };
 
-        Ok(PackageJson::new(path.to_path_buf(), value))
+        Ok(PackageJson { path, value })
     }
 
     pub fn save(&mut self) -> Result<(), PackageJsonError> {
         let mut file = fs::File::create(&self.path)?;
-        let contents = PackageJson::to_string_prettify(&self.value)?;
+        let contents = serde_json::to_string_pretty(&self.value)?;
         file.write_all(contents.as_bytes())?;
         Ok(())
     }
@@ -213,33 +205,15 @@ mod tests {
 
     #[test]
     fn test_init_package_json_content() {
-        let package_json = PackageJson::get_init_package_json("test").unwrap();
-        assert_snapshot!(PackageJson::to_string_prettify(&package_json).unwrap_or(String::new()));
-    }
-
-    #[test]
-    fn test_dependency_group_into() {
-        assert_eq!(<DependencyGroup as Into<&str>>::into(DependencyGroup::Default), "dependencies");
-        assert_eq!(<DependencyGroup as Into<&str>>::into(DependencyGroup::Dev), "devDependencies");
-        assert_eq!(
-            <DependencyGroup as Into<&str>>::into(DependencyGroup::Optional),
-            "optionalDependencies"
-        );
-        assert_eq!(
-            <DependencyGroup as Into<&str>>::into(DependencyGroup::Peer),
-            "peerDependencies"
-        );
-        assert_eq!(
-            <DependencyGroup as Into<&str>>::into(DependencyGroup::Bundled),
-            "bundledDependencies"
-        );
+        let package_json = PackageJson::get_init_package_json("test");
+        assert_snapshot!(serde_json::to_string_pretty(&package_json).unwrap());
     }
 
     #[test]
     fn init_should_throw_if_exists() {
         let tmp = NamedTempFile::new().unwrap();
         write!(tmp.as_file(), "hello world").unwrap();
-        PackageJson::init(&tmp.path().to_path_buf()).expect_err("package.json already exist");
+        PackageJson::init(tmp.path()).expect_err("package.json already exist");
     }
 
     #[test]
@@ -249,14 +223,14 @@ mod tests {
         PackageJson::init(&tmp).unwrap();
         assert!(tmp.exists());
         assert!(tmp.is_file());
-        assert_eq!(PackageJson::from_path(&tmp).unwrap().path, tmp);
+        assert_eq!(PackageJson::from_path(tmp.clone()).unwrap().path, tmp);
     }
 
     #[test]
     fn should_add_dependency() {
         let dir = tempdir().unwrap();
         let tmp = dir.path().join("package.json");
-        let mut package_json = PackageJson::create_if_needed(&tmp).unwrap();
+        let mut package_json = PackageJson::create_if_needed(tmp.clone()).unwrap();
         package_json.add_dependency("fastify", "1.0.0", DependencyGroup::Default).unwrap();
 
         let dependencies = package_json.get_dependencies(&[DependencyGroup::Default]);
@@ -270,7 +244,7 @@ mod tests {
     fn should_throw_on_missing_command() {
         let dir = tempdir().unwrap();
         let tmp = dir.path().join("package.json");
-        let package_json = PackageJson::create_if_needed(&tmp).unwrap();
+        let package_json = PackageJson::create_if_needed(tmp).unwrap();
         package_json.get_script("dev", false).expect_err("dev command should not exist");
     }
 
@@ -285,7 +259,7 @@ mod tests {
         "#;
         let tmp = NamedTempFile::new().unwrap();
         write!(tmp.as_file(), "{}", data).unwrap();
-        let package_json = PackageJson::create_if_needed(&tmp.path().to_path_buf()).unwrap();
+        let package_json = PackageJson::create_if_needed(tmp.path().to_path_buf()).unwrap();
         package_json.get_script("test", false).unwrap();
         package_json.get_script("invalid", false).expect_err("invalid command should not exist");
         package_json.get_script("invalid", true).unwrap();
@@ -305,7 +279,7 @@ mod tests {
         "#;
         let tmp = NamedTempFile::new().unwrap();
         write!(tmp.as_file(), "{}", data).unwrap();
-        let package_json = PackageJson::create_if_needed(&tmp.path().to_path_buf()).unwrap();
+        let package_json = PackageJson::create_if_needed(tmp.path().to_path_buf()).unwrap();
         assert!(package_json
             .get_dependencies(&[DependencyGroup::Peer])
             .contains_key("fast-querystring"));

crates/package_json/src/snapshots/pacquet_package_json__tests__init_package_json_content.snap

@@ -1,6 +1,6 @@
 ---
 source: crates/package_json/src/lib.rs
-expression: "PackageJson::to_string_prettify(&package_json).unwrap_or(String::new())"
+expression: "serde_json::to_string_pretty(&package_json).unwrap()"
 ---
 {
   "name": "test",