Merge pull request #1020 from bakkot/install-version-number

Add error for `volta install 10`

Author: charlespierce

Cargo.lock

@@ -163,6 +163,13 @@ pub enum ErrorKind {
         version: String,
     },
 
+    /// Thrown when a user does e.g. `volta install 12` instead of
+    /// `volta install node@12`.
+    InvalidInvocationOfBareVersion {
+        action: String,
+        version: String,
+    },
+
     /// Thrown when a tool name is invalid per npm's rules.
     InvalidToolName {
         name: String,
@@ -770,6 +777,32 @@ To {action} the packages '{name}' and '{version}', please {action} them in separ
                 write!(f, "{}\n\n{}", error, wrapped_cta)
             }
 
+            ErrorKind::InvalidInvocationOfBareVersion {
+                action,
+                version,
+            } => {
+                let error = format!(
+                    "`volta {action} {version}` is not supported.",
+                    action = action,
+                    version = version
+                );
+
+                let call_to_action = format!(
+"To {action} node version '{version}', please run `volta {action} {formatted}`. \
+To {action} the package '{version}', please use an explicit version such as '{version}@latest'.",
+                    action=action,
+                    version=version,
+                    formatted=tool_version("node", version)
+                );
+
+                let wrapped_cta = match text_width() {
+                    Some(width) => fill(&call_to_action, width),
+                    None => call_to_action,
+                };
+
+                write!(f, "{}\n\n{}", error, wrapped_cta)
+            }
+
             ErrorKind::InvalidToolName { name, errors } => {
                 let indentation = "    ";
                 let wrapped = match text_width() {
@@ -1383,6 +1416,7 @@ impl ErrorKind {
             ErrorKind::InvalidHookCommand { .. } => ExitCode::ExecutableNotFound,
             ErrorKind::InvalidHookOutput { .. } => ExitCode::ExecutionFailure,
             ErrorKind::InvalidInvocation { .. } => ExitCode::InvalidArguments,
+            ErrorKind::InvalidInvocationOfBareVersion { .. } => ExitCode::InvalidArguments,
             ErrorKind::InvalidToolName { .. } => ExitCode::InvalidArguments,
             ErrorKind::LockAcquireError => ExitCode::FileSystemError,
             ErrorKind::NoBundledNpm { .. } => ExitCode::ConfigurationError,

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

@@ -83,33 +83,50 @@ impl Spec {
         Ok(tools)
     }
 
-    /// Check the args for the bad pattern of `volta install <tool> <number>`.
+    /// Check the args for the bad patterns of
+    /// - `volta install <number>`
+    /// - `volta install <tool> <number>`
     fn check_args<T>(args: &[T], action: &str) -> Fallible<()>
     where
         T: AsRef<str>,
     {
         let mut args = args.iter();
 
-        // The case we are concerned with is where we have `<tool> <number>`.
-        // This is only interesting if there are exactly two args. Then we care
-        // whether the two items are a bare name (with no `@version`), followed
-        // by a valid version specifier (ignoring custom tags). That is:
-        //
-        // - `volta install node@lts latest` is allowed.
-        // - `volta install node latest` is an error.
-        // - `volta install node latest yarn` is allowed.
-        if let (Some(name), Some(maybe_version), None) = (args.next(), args.next(), args.next()) {
-            if !HAS_VERSION.is_match(name.as_ref()) && is_version_like(maybe_version.as_ref()) {
-                return Err(ErrorKind::InvalidInvocation {
+        match (args.next(), args.next(), args.next()) {
+            // The case we are concerned with here is where we have `<number>`.
+            // That is, exactly one argument, which is a valid version specifier.
+            //
+            // - `volta install node@12` is allowed.
+            // - `volta install 12` is an error.
+            // - `volta install lts` is an error.
+            (Some(maybe_version), None, None) if is_version_like(maybe_version.as_ref()) => {
+                Err(ErrorKind::InvalidInvocationOfBareVersion {
+                    action: action.to_string(),
+                    version: maybe_version.as_ref().to_string(),
+                }
+                .into())
+            }
+            // The case we are concerned with here is where we have `<tool> <number>`.
+            // This is only interesting if there are exactly two args. Then we care
+            // whether the two items are a bare name (with no `@version`), followed
+            // by a valid version specifier (ignoring custom tags). That is:
+            //
+            // - `volta install node@lts latest` is allowed.
+            // - `volta install node latest` is an error.
+            // - `volta install node latest yarn` is allowed.
+            (Some(name), Some(maybe_version), None)
+                if !HAS_VERSION.is_match(name.as_ref())
+                    && is_version_like(maybe_version.as_ref()) =>
+            {
+                Err(ErrorKind::InvalidInvocation {
                     action: action.to_string(),
                     name: name.as_ref().to_string(),
                     version: maybe_version.as_ref().to_string(),
                 }
-                .into());
+                .into())
             }
+            _ => Ok(()),
         }
-
-        Ok(())
     }
 
     /// Compare `Spec`s for sorting when converting from strings
@@ -358,6 +375,23 @@ mod tests {
 
         static PIN: &str = "pin";
 
+        #[test]
+        fn special_cases_just_number() {
+            let version = "1.2.3";
+            let args: Vec<String> = vec![version.into()];
+
+            let err = Spec::from_strings(&args, PIN).unwrap_err();
+
+            assert_eq!(
+                err.kind(),
+                &ErrorKind::InvalidInvocationOfBareVersion {
+                    action: PIN.into(),
+                    version: version.into()
+                },
+                "`volta <action> number` results in the correct error"
+            );
+        }
+
         #[test]
         fn special_cases_tool_space_number() {
             let name = "potato";
@@ -393,6 +427,15 @@ mod tests {
                 "when there is only one arg"
             );
 
+            let one_with_explicit_verson = ["10@latest".to_owned()];
+            assert_eq!(
+                Spec::from_strings(&one_with_explicit_verson, PIN)
+                    .expect("is ok")
+                    .len(),
+                only_one.len(),
+                "when the sole arg is version-like but has an explicit version"
+            );
+
             let two_but_unmistakable = ["12".to_owned(), "node".to_owned()];
             assert_eq!(
                 Spec::from_strings(&two_but_unmistakable, PIN)