Switch Node filename builder to use Version object

Author: charlespierce

crates/volta-core/src/tool/node/fetch.rs

@@ -30,7 +30,7 @@ cfg_if! {
     }
 }
 
-fn npm_manifest_path(version: &str) -> PathBuf {
+fn npm_manifest_path(version: &Version) -> PathBuf {
     let mut manifest = PathBuf::from(Node::archive_basename(version));
 
     #[cfg(unix)]
@@ -46,7 +46,7 @@ fn npm_manifest_path(version: &str) -> PathBuf {
 pub fn fetch(version: &Version, hooks: Option<&ToolHooks<Node>>) -> Fallible<NodeVersion> {
     let home = volta_home()?;
     let node_dir = home.node_inventory_dir();
-    let cache_file = node_dir.join(Node::archive_filename(&version.to_string()));
+    let cache_file = node_dir.join(Node::archive_filename(version));
 
     let (archive, staging) = match load_cached_distro(&cache_file) {
         Some(archive) => {
@@ -107,22 +107,20 @@ fn unpack_archive(archive: Box<dyn Archive>, version: &Version) -> Fallible<Node
         })?;
 
     // Save the npm version number in the npm version file for this distro
-    let npm_package_json = temp.path().join(npm_manifest_path(&version_string));
+    let npm_package_json = temp.path().join(npm_manifest_path(version));
     let npm = Manifest::version(&npm_package_json)?;
     save_default_npm_version(&version, &npm)?;
 
     let dest = volta_home()?.node_image_dir(&version_string);
     ensure_containing_dir_exists(&dest)
         .with_context(|| ErrorKind::ContainingDirError { path: dest.clone() })?;
 
-    rename(
-        temp.path().join(Node::archive_basename(&version_string)),
-        &dest,
-    )
-    .with_context(|| ErrorKind::SetupToolImageError {
-        tool: "Node".into(),
-        version: version_string,
-        dir: dest.clone(),
+    rename(temp.path().join(Node::archive_basename(version)), &dest).with_context(|| {
+        ErrorKind::SetupToolImageError {
+            tool: "Node".into(),
+            version: version_string,
+            dir: dest.clone(),
+        }
     })?;
 
     progress.finish_and_clear();
@@ -151,8 +149,7 @@ fn load_cached_distro(file: &Path) -> Option<Box<dyn Archive>> {
 
 /// Determine the remote URL to download from, using the hooks if available
 fn determine_remote_url(version: &Version, hooks: Option<&ToolHooks<Node>>) -> Fallible<String> {
-    let version_str = version.to_string();
-    let distro_file_name = Node::archive_filename(&version_str);
+    let distro_file_name = Node::archive_filename(version);
     match hooks {
         Some(&ToolHooks {
             distro: Some(ref hook),

crates/volta-core/src/tool/node/mod.rs

@@ -129,11 +129,11 @@ impl Node {
         Node { version }
     }
 
-    pub fn archive_basename(version: &str) -> String {
+    pub fn archive_basename(version: &Version) -> String {
         format!("node-v{}-{}-{}", version, NODE_DISTRO_OS, NODE_DISTRO_ARCH)
     }
 
-    pub fn archive_filename(version: &str) -> String {
+    pub fn archive_filename(version: &Version) -> String {
         format!(
             "{}.{}",
             Node::archive_basename(version),
@@ -242,15 +242,15 @@ mod tests {
     #[test]
     fn test_node_archive_basename() {
         assert_eq!(
-            Node::archive_basename("1.2.3"),
+            Node::archive_basename(&Version::new(1, 2, 3)),
             format!("node-v1.2.3-{}-{}", NODE_DISTRO_OS, NODE_DISTRO_ARCH)
         );
     }
 
     #[test]
     fn test_node_archive_filename() {
         assert_eq!(
-            Node::archive_filename("1.2.3"),
+            Node::archive_filename(&Version::new(1, 2, 3)),
             format!(
                 "node-v1.2.3-{}-{}.{}",
                 NODE_DISTRO_OS, NODE_DISTRO_ARCH, NODE_DISTRO_EXTENSION

tests/acceptance/corrupted_download.rs

@@ -1,6 +1,7 @@
 use crate::support::sandbox::{sandbox, DistroMetadata, NodeFixture, YarnFixture};
 use hamcrest2::assert_that;
 use hamcrest2::prelude::*;
+use semver::Version;
 use test_support::matchers::execs;
 
 use volta_core::error::ExitCode;
@@ -58,7 +59,7 @@ fn install_corrupted_node_leaves_inventory_unchanged() {
         execs().with_status(ExitCode::UnknownError as i32)
     );
 
-    assert!(!s.node_inventory_archive_exists("0.0.1"));
+    assert!(!s.node_inventory_archive_exists(&Version::new(0, 0, 1)));
 }
 
 #[test]
@@ -73,7 +74,7 @@ fn install_valid_node_saves_to_inventory() {
         execs().with_status(ExitCode::Success as i32)
     );
 
-    assert!(s.node_inventory_archive_exists("10.99.1040"));
+    assert!(s.node_inventory_archive_exists(&Version::new(10, 99, 1040)));
 }
 
 #[test]

tests/acceptance/support/sandbox.rs

@@ -7,6 +7,7 @@ use std::time::{Duration, SystemTime};
 
 use hyperx::header::HttpDate;
 use mockito::{self, mock, Matcher};
+use semver::Version;
 use test_support::{self, ok_or_panic, paths, paths::PathExt, process::ProcessBuilder};
 use volta_core::fs::symlink_file;
 use volta_core::tool::{Node, Yarn, NODE_DISTRO_ARCH, NODE_DISTRO_EXTENSION, NODE_DISTRO_OS};
@@ -631,7 +632,7 @@ impl Sandbox {
 
     // check that files in the sandbox exist
 
-    pub fn node_inventory_archive_exists(&self, version: &str) -> bool {
+    pub fn node_inventory_archive_exists(&self, version: &Version) -> bool {
         node_inventory_dir()
             .join(Node::archive_filename(version))
             .exists()