address PR feedback

Author: mikrostew

crates/volta-core/src/error/kind.rs

@@ -1379,9 +1379,9 @@ Please ensure you have permissions to edit your environment variables."
             ),
             ErrorKind::Yarn2NotSupported => write!(
                 f,
-                "Yarn@2 is not recommended for use, and not supported by Volta.
+                "Yarn version 2 is not recommended for use, and not supported by Volta.
 
-Please use Yarn@3 instead."
+Please use version 3 or greater instead."
             ),
             ErrorKind::YarnLatestFetchError { from_url } => write!(
                 f,

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

@@ -129,7 +129,7 @@ fn determine_remote_url(version: &Version, hooks: Option<&ToolHooks<Npm>>) -> Fa
             let distro_file_name = Npm::archive_filename(&version_str);
             hook.resolve(version, &distro_file_name)
         }
-        _ => Ok(public_registry_package("npm", "npm", &version_str)),
+        _ => Ok(public_registry_package("npm", &version_str)),
     }
 }
 

crates/volta-core/src/tool/npm/resolve.rs

@@ -1,18 +1,13 @@
 //! Provides resolution of npm Version requirements into specific versions
 
 use super::super::registry::{
-    public_registry_index, PackageDetails, PackageIndex, RawPackageMetadata,
-    NPM_ABBREVIATED_ACCEPT_HEADER,
+    fetch_public_index, public_registry_index, PackageDetails, PackageIndex,
 };
-use super::super::registry_fetch_error;
-use crate::error::{Context, ErrorKind, Fallible};
+use crate::error::{ErrorKind, Fallible};
 use crate::hook::ToolHooks;
 use crate::session::Session;
-use crate::style::progress_spinner;
 use crate::tool::Npm;
 use crate::version::{VersionSpec, VersionTag};
-use attohttpc::header::ACCEPT;
-use attohttpc::Response;
 use log::debug;
 use semver::{Version, VersionReq};
 
@@ -41,16 +36,7 @@ fn fetch_npm_index(hooks: Option<&ToolHooks<Npm>>) -> Fallible<(String, PackageI
         _ => public_registry_index("npm"),
     };
 
-    let spinner = progress_spinner(format!("Fetching public registry: {}", url));
-    let metadata: RawPackageMetadata = attohttpc::get(&url)
-        .header(ACCEPT, NPM_ABBREVIATED_ACCEPT_HEADER)
-        .send()
-        .and_then(Response::error_for_status)
-        .and_then(Response::json)
-        .with_context(registry_fetch_error("npm", &url))?;
-
-    spinner.finish_and_clear();
-    Ok((url, metadata.into()))
+    fetch_public_index(url, "Npm")
 }
 
 fn resolve_tag(tag: &str, hooks: Option<&ToolHooks<Npm>>) -> Fallible<Version> {

crates/volta-core/src/tool/registry.rs

@@ -1,9 +1,13 @@
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 
+use super::registry_fetch_error;
 use crate::error::{Context, ErrorKind, Fallible};
 use crate::fs::read_dir_eager;
+use crate::style::progress_spinner;
 use crate::version::{hashmap_version_serde, version_serde};
+use attohttpc::header::ACCEPT;
+use attohttpc::Response;
 use cfg_if::cfg_if;
 use semver::Version;
 use serde::Deserialize;
@@ -30,13 +34,36 @@ cfg_if! {
     }
 }
 
-// need package and filename for namespaced tools like @yarnpkg/cli-dist, which is located at
-//   https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-1.2.3.tgz
-pub fn public_registry_package(package: &str, filename: &str, version: &str) -> String {
+pub fn fetch_public_index(url: String, name: &str) -> Fallible<(String, PackageIndex)> {
+    let spinner = progress_spinner(format!("Fetching public registry: {}", url));
+    let metadata: RawPackageMetadata = attohttpc::get(&url)
+        .header(ACCEPT, NPM_ABBREVIATED_ACCEPT_HEADER)
+        .send()
+        .and_then(Response::error_for_status)
+        .and_then(Response::json)
+        .with_context(registry_fetch_error(name, &url))?;
+
+    spinner.finish_and_clear();
+    Ok((url, metadata.into()))
+}
+
+pub fn public_registry_package(package: &str, version: &str) -> String {
     format!(
         "{}/-/{}-{}.tgz",
         public_registry_index(package),
-        filename,
+        package,
+        version
+    )
+}
+
+// need package and filename for namespaced tools like @yarnpkg/cli-dist, which is located at
+//   https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-1.2.3.tgz
+pub fn scoped_public_registry_package(scope: &str, package: &str, version: &str) -> String {
+    format!(
+        "{}/{}/-/{}-{}.tgz",
+        public_registry_index(scope),
+        package,
+        package,
         version
     )
 }

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

@@ -5,7 +5,9 @@ use std::fs::File;
 use std::path::Path;
 
 use super::super::download_tool_error;
-use super::super::registry::{find_unpack_dir, public_registry_package};
+use super::super::registry::{
+    find_unpack_dir, public_registry_package, scoped_public_registry_package,
+};
 use crate::error::{Context, ErrorKind, Fallible};
 use crate::fs::{create_staging_dir, create_staging_file, rename, set_executable};
 use crate::hook::ToolHooks;
@@ -16,7 +18,7 @@ use crate::version::VersionSpec;
 use archive::{Archive, Tarball};
 use fs_utils::ensure_containing_dir_exists;
 use log::debug;
-use semver::{Version, VersionReq};
+use semver::Version;
 
 pub fn fetch(version: &Version, hooks: Option<&ToolHooks<Yarn>>) -> Fallible<()> {
     let yarn_dir = volta_home()?.yarn_inventory_dir();
@@ -127,16 +129,14 @@ fn determine_remote_url(version: &Version, hooks: Option<&ToolHooks<Yarn>>) -> F
             hook.resolve(version, &distro_file_name)
         }
         _ => {
-            let matches_yarn_berry = VersionReq::parse(">=2").unwrap();
-            if env::var_os("VOLTA_FEATURE_YARN_3").is_some() && matches_yarn_berry.matches(version)
-            {
-                Ok(public_registry_package(
-                    "@yarnpkg/cli-dist",
+            if env::var_os("VOLTA_FEATURE_YARN_3").is_some() && version.major >= 2 {
+                Ok(scoped_public_registry_package(
+                    "@yarnpkg",
                     "cli-dist",
                     &version_str,
                 ))
             } else {
-                Ok(public_registry_package("yarn", "yarn", &version_str))
+                Ok(public_registry_package("yarn", &version_str))
             }
         }
     }

crates/volta-core/src/tool/yarn/resolve.rs

@@ -3,8 +3,7 @@
 use std::env;
 
 use super::super::registry::{
-    public_registry_index, PackageDetails, PackageIndex, RawPackageMetadata,
-    NPM_ABBREVIATED_ACCEPT_HEADER,
+    fetch_public_index, public_registry_index, PackageDetails, PackageIndex,
 };
 use super::super::registry_fetch_error;
 use super::metadata::{RawYarnIndex, YarnIndex};
@@ -14,7 +13,6 @@ use crate::session::Session;
 use crate::style::progress_spinner;
 use crate::tool::Yarn;
 use crate::version::{parse_version, VersionSpec, VersionTag};
-use attohttpc::header::ACCEPT;
 use attohttpc::Response;
 use log::debug;
 use semver::{Version, VersionReq};
@@ -73,27 +71,17 @@ fn resolve_semver(matching: VersionReq, hooks: Option<&ToolHooks<Yarn>>) -> Fall
 
 fn fetch_yarn_index(package: &str) -> Fallible<(String, PackageIndex)> {
     let url = public_registry_index(package);
-    let spinner = progress_spinner(format!("Fetching public registry: {}", url));
-    let metadata: RawPackageMetadata = attohttpc::get(&url)
-        .header(ACCEPT, NPM_ABBREVIATED_ACCEPT_HEADER)
-        .send()
-        .and_then(Response::error_for_status)
-        .and_then(Response::json)
-        .with_context(registry_fetch_error("Yarn", &url))?;
-
-    spinner.finish_and_clear();
-    Ok((url, metadata.into()))
+    fetch_public_index(url, "Yarn")
 }
 
 fn resolve_custom_tag(tag: String) -> Fallible<Version> {
     if env::var_os("VOLTA_FEATURE_YARN_3").is_some() {
         // first try yarn2+, which uses "@yarnpkg/cli-dist" instead of "yarn"
         let (url, mut index) = fetch_yarn_index("@yarnpkg/cli-dist")?;
 
-        let matches_yarn_2 = VersionReq::parse("2.*").unwrap();
         if let Some(version) = index.tags.remove(&tag) {
             debug!("Found yarn@{} matching tag '{}' from {}", version, tag, url);
-            if matches_yarn_2.matches(&version) {
+            if version.major == 2 {
                 return Err(ErrorKind::Yarn2NotSupported.into());
             }
             return Ok(version);
@@ -137,11 +125,10 @@ fn resolve_semver_from_registry(matching: VersionReq) -> Fallible<Version> {
             .filter(|PackageDetails { version, .. }| matching.matches(version))
             .collect();
 
-        if matching_entries.len() > 0 {
-            let matches_yarn_3 = VersionReq::parse(">=3").unwrap();
+        if !matching_entries.is_empty() {
             let details_opt = matching_entries
                 .iter()
-                .find(|PackageDetails { version, .. }| matches_yarn_3.matches(version));
+                .find(|PackageDetails { version, .. }| version.major >= 3);
 
             match details_opt {
                 Some(details) => {

tests/acceptance/volta_pin.rs

@@ -425,7 +425,7 @@ fn pin_yarn_2_is_error() {
         execs()
             .with_status(ExitCode::NoVersionMatch as i32)
             .with_stderr_contains(
-                "[..]Yarn@2 is not recommended for use, and not supported by Volta[..]"
+                "[..]Yarn version 2 is not recommended for use, and not supported by Volta[..]"
             )
     );