Repository: taiki-e/cargo-hack (rust)

Issue:
`--remove-dev-deps` does not work properly without subcommand
This is a regression due to https://github.com/taiki-e/cargo-hack/commit/12534df1509e4a2a20749288f81414bedbdcacec.

If there is no subcommand then all packages will be skipped:

https://github.com/taiki-e/cargo-hack/blob/6d2227259fb0dd4495c583983f54198084a27453/src/package.rs#L30-L35

https://github.com/taiki-e/cargo-hack/blob/6d2227259fb0dd4495c583983f54198084a27453/src/package.rs#L46

Pull request:
Refactor package.rs
Fixes #35

Patch:
diff --git a/src/main.rs b/src/main.rs
index 9eed164..dc70dd3 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -50,6 +50,11 @@ fn try_main(coloring: &mut Option<Coloring>) -> Result<()> {
 }
 
 fn exec_on_workspace(args: &Args, current_manifest: &Manifest, metadata: &Metadata) -> Result<()> {
+    assert!(
+        args.subcommand.is_some() || args.remove_dev_deps,
+        "no subcommand or valid flag specified"
+    );
+
     let restore = Restore::new(args);
     let mut line = ProcessBuilder::from_args(cargo_binary(), &args);
 
@@ -112,18 +117,17 @@ fn exec_on_package(
     restore: &Restore,
     info: &mut Info,
 ) -> Result<()> {
-    if let Kind::Skip = package.kind {
-        info!(args.color, "skipped running on {}", package.name_verbose(args));
-    } else if args.subcommand.is_some() || args.remove_dev_deps {
+    if let Kind::SkipAsPrivate = package.kind {
+        info!(args.color, "skipped running on private crate {}", package.name_verbose(args));
+        Ok(())
+    } else {
         let mut line = line.clone();
         line.features(args, package);
         line.arg("--manifest-path");
         line.arg(&package.manifest_path);
 
-        no_dev_deps(args, package, &line, restore, info)?;
+        no_dev_deps(args, package, &line, restore, info)
     }
-
-    Ok(())
 }
 
 fn no_dev_deps(
@@ -141,16 +145,12 @@ fn no_dev_deps(
             format!("failed to update manifest file: {}", package.manifest_path.display())
         })?;
 
-        if args.subcommand.is_some() {
-            package::features(args, package, line, info)?;
-        }
+        package::exec(args, package, line, info)?;
 
-        handle.done()?;
-    } else if args.subcommand.is_some() {
-        package::features(args, package, line, info)?;
+        handle.done()
+    } else {
+        package::exec(args, package, line, info)
     }
-
-    Ok(())
 }
 
 fn cargo_binary() -> OsString {
diff --git a/src/package.rs b/src/package.rs
index 3ea8451..dad0c2a 100644
--- a/src/package.rs
+++ b/src/package.rs
@@ -2,15 +2,6 @@ use std::{fmt::Write, ops::Deref};
 
 use crate::{metadata, Args, Info, Manifest, ProcessBuilder, Result};
 
-pub(crate) fn features(
-    args: &Args,
-    package: &Package<'_>,
-    line: &ProcessBuilder,
-    info: &mut Info,
-) -> Result<()> {
-    Features { args, package, line: line.clone(), info }.exec()
-}
-
 pub(crate) struct Package<'a> {
     package: &'a metadata::Package,
     pub(crate) manifest: Manifest,
@@ -18,21 +9,13 @@ pub(crate) struct Package<'a> {
 }
 
 impl<'a> Package<'a> {
-    fn new(
-        args: &'a Args,
-        package: &'a metadata::Package,
-        total: &mut usize,
-    ) -> Result<Option<Self>> {
+    fn new(args: &'a Args, package: &'a metadata::Package, total: &mut usize) -> Result<Self> {
         let manifest = Manifest::new(&package.manifest_path)?;
 
         if args.ignore_private && manifest.is_private() {
-            Ok(Some(Self { package, manifest, kind: Kind::Skip }))
-        } else if args.subcommand.is_some() {
-            let (kind, count) = Kind::collect(args, package);
-            *total += count;
-            Ok(Some(Self { package, manifest, kind }))
+            Ok(Self { package, manifest, kind: Kind::SkipAsPrivate })
         } else {
-            Ok(None)
+            Ok(Self { package, manifest, kind: Kind::determine(args, package, total) })
         }
     }
 
@@ -43,7 +26,7 @@ impl<'a> Package<'a> {
     ) -> Result<Vec<Self>> {
         packages
             .into_iter()
-            .filter_map(|package| Package::new(args, package, total).transpose())
+            .map(|package| Package::new(args, package, total))
             .collect::<Result<Vec<_>>>()
     }
 }
@@ -57,16 +40,23 @@ impl Deref for Package<'_> {
 }
 
 pub(crate) enum Kind<'a> {
-    Skip,
+    // If there is no subcommand, then kind need not be determined.
+    NoSubcommand,
+    SkipAsPrivate,
     Nomal { show_progress: bool },
     Each { features: Vec<&'a String> },
     Powerset { features: Vec<Vec<&'a String>> },
 }
 
 impl<'a> Kind<'a> {
-    fn collect(args: &'a Args, package: &'a metadata::Package) -> (Self, usize) {
+    fn determine(args: &'a Args, package: &'a metadata::Package, total: &mut usize) -> Self {
+        if args.subcommand.is_none() {
+            return Kind::NoSubcommand;
+        }
+
         if !args.each_feature && !args.feature_powerset {
-            return (Kind::Nomal { show_progress: false }, 1);
+            *total += 1;
+            return Kind::Nomal { show_progress: false };
         }
 
         let features =
@@ -85,13 +75,14 @@ impl<'a> Kind<'a> {
             };
 
             if package.features.is_empty() && features.is_empty() {
-                return (Kind::Nomal { show_progress: true }, 1);
+                *total += 1;
+                Kind::Nomal { show_progress: true }
+            } else {
+                // +1: default features
+                // +1: no default features
+                *total += features.len() + 2;
+                Kind::Each { features }
             }
-
-            // +1: default features
-            // +1: no default features
-            let total = features.len() + 2;
-            (Kind::Each { features }, total)
         } else if args.feature_powerset {
             let features = if let Some(opt_deps) = opt_deps {
                 powerset(features.chain(opt_deps))
@@ -100,80 +91,95 @@ impl<'a> Kind<'a> {
             };
 
             if package.features.is_empty() && features.is_empty() {
-                return (Kind::Nomal { show_progress: true }, 1);
+                *total += 1;
+                Kind::Nomal { show_progress: true }
+            } else {
+                // +1: default features
+                // +1: no default features
+                // -1: the first element of a powerset is `[]`
+                *total += features.len() + 1;
+                Kind::Powerset { features }
             }
-
-            // +1: default features
-            // +1: no default features
-            // -1: the first element of a powerset is `[]`
-            let total = features.len() + 1;
-            (Kind::Powerset { features }, total)
         } else {
             unreachable!()
         }
     }
 }
 
-struct Features<'a> {
-    args: &'a Args,
-    package: &'a Package<'a>,
-    line: ProcessBuilder,
-    info: &'a mut Info,
-}
-
-impl Features<'_> {
-    fn exec(&mut self) -> Result<()> {
-        if let Kind::Nomal { show_progress } = &self.package.kind {
-            // run with default features
-            return self.exec_cargo(None, *show_progress);
-        }
-
-        // run with default features
-        self.exec_cargo(None, true)?;
-
-        self.line.arg("--no-default-features");
-
-        // run with no default features if the package has other features
-        //
-        // `default` is not skipped because `cfg(feature = "default")` is work
-        // if `default` feature specified.
-        self.exec_cargo(None, true)?;
-
-        match &self.package.kind {
-            Kind::Each { features } => {
-                features.iter().try_for_each(|f| self.exec_cargo_with_features(Some(f)))
-            }
-            Kind::Powerset { features } => {
-                // The first element of a powerset is `[]` so it should be skipped.
-                features.iter().skip(1).try_for_each(|f| self.exec_cargo_with_features(f))
-            }
-            _ => unreachable!(),
+pub(crate) fn exec(
+    args: &Args,
+    package: &Package<'_>,
+    line: &ProcessBuilder,
+    info: &mut Info,
+) -> Result<()> {
+    match &package.kind {
+        Kind::NoSubcommand => return Ok(()),
+        Kind::SkipAsPrivate => unreachable!(),
+        Kind::Nomal { show_progress } => {
+            // only run with default features
+            return exec_cargo(args, package, &line, info, *show_progress);
         }
+        Kind::Each { .. } | Kind::Powerset { .. } => {}
     }
 
-    fn exec_cargo_with_features(
-        &mut self,
-        features: impl IntoIterator<Item = impl AsRef<str>>,
-    ) -> Result<()> {
-        let mut line = self.line.clone();
-        line.append_features(features);
-        self.exec_cargo(Some(&line), true)
+    let mut line = line.clone();
+
+    // run with default features
+    exec_cargo(args, package, &line, info, true)?;
+
+    line.arg("--no-default-features");
+
+    // run with no default features if the package has other features
+    //
+    // `default` is not skipped because `cfg(feature = "default")` is work
+    // if `default` feature specified.
+    exec_cargo(args, package, &line, info, true)?;
+
+    match &package.kind {
+        Kind::Each { features } => features
+            .iter()
+            .try_for_each(|f| exec_cargo_with_features(args, package, &line, info, Some(f))),
+        Kind::Powerset { features } => {
+            // The first element of a powerset is `[]` so it should be skipped.
+            features
+                .iter()
+                .skip(1)
+                .try_for_each(|f| exec_cargo_with_features(args, package, &line, info, f))
+        }
+        _ => unreachable!(),
     }
+}
 
-    fn exec_cargo(&mut self, line: Option<&ProcessBuilder>, show_progress: bool) -> Result<()> {
-        let line = line.unwrap_or(&self.line);
-        self.info.count += 1;
+fn exec_cargo_with_features(
+    args: &Args,
+    package: &Package<'_>,
+    line: &ProcessBuilder,
+    info: &mut Info,
+    features: impl IntoIterator<Item = impl AsRef<str>>,
+) -> Result<()> {
+    let mut line = line.clone();
+    line.append_features(features);
+    exec_cargo(args, package, &line, info, true)
+}
 
-        // running `<command>` on <package> (<count>/<total>)
-        let mut msg = String::new();
-        write!(msg, "running {} on {}", line, &self.package.name).unwrap();
-        if show_progress {
-            write!(msg, " ({}/{})", self.info.count, self.info.total).unwrap();
-        }
-        info!(self.args.color, "{}", msg);
+fn exec_cargo(
+    args: &Args,
+    package: &Package<'_>,
+    line: &ProcessBuilder,
+    info: &mut Info,
+    show_progress: bool,
+) -> Result<()> {
+    info.count += 1;
 
-        line.exec()
+    // running `<command>` on <package> (<count>/<total>)
+    let mut msg = String::new();
+    write!(msg, "running {} on {}", line, &package.name).unwrap();
+    if show_progress {
+        write!(msg, " ({}/{})", info.count, info.total).unwrap();
     }
+    info!(args.color, "{}", msg);
+
+    line.exec()
 }
 
 fn powerset<'a, T>(iter: impl IntoIterator<Item = &'a T>) -> Vec<Vec<&'a T>> {


Test patch:
diff --git a/tests/test.rs b/tests/test.rs
index 2d3b592..27dbcb5 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -238,11 +238,11 @@ fn real_ignore_private() {
         .unwrap()
         .assert_success()
         .assert_stderr_not_contains("running `cargo check` on member1")
-        .assert_stderr_not_contains("skipped running on member1")
+        .assert_stderr_not_contains("skipped running on private crate member1")
         .assert_stderr_not_contains("running `cargo check` on member2")
-        .assert_stderr_not_contains("skipped running on member2")
+        .assert_stderr_not_contains("skipped running on private crate member2")
         .assert_stderr_not_contains("running `cargo check` on real")
-        .assert_stderr_contains("skipped running on real");
+        .assert_stderr_contains("skipped running on private crate real");
 
     cargo_hack()
         .args(&["check", "--all", "--ignore-private"])
@@ -251,13 +251,13 @@ fn real_ignore_private() {
         .unwrap()
         .assert_success()
         .assert_stderr_contains("running `cargo check` on member1")
-        .assert_stderr_not_contains("skipped running on member1")
+        .assert_stderr_not_contains("skipped running on private crate member1")
         .assert_stderr_not_contains("running `cargo check` on member2")
-        .assert_stderr_contains("skipped running on member2")
+        .assert_stderr_contains("skipped running on private crate member2")
         .assert_stderr_contains("running `cargo check` on member3")
-        .assert_stderr_not_contains("skipped running on member3")
+        .assert_stderr_not_contains("skipped running on private crate member3")
         .assert_stderr_not_contains("running `cargo check` on real")
-        .assert_stderr_contains("skipped running on real");
+        .assert_stderr_contains("skipped running on private crate real");
 }
 
 #[test]
@@ -270,8 +270,8 @@ fn virtual_ignore_private() {
         .assert_success()
         .assert_stderr_contains("running `cargo check` on member1")
         .assert_stderr_not_contains("running `cargo check` on member2")
-        .assert_stderr_not_contains("skipped running on member1")
-        .assert_stderr_contains("skipped running on member2");
+        .assert_stderr_not_contains("skipped running on private crate member1")
+        .assert_stderr_contains("skipped running on private crate member2");
 
     cargo_hack()
         .args(&["check", "--all", "--ignore-private"])
@@ -281,8 +281,8 @@ fn virtual_ignore_private() {
         .assert_success()
         .assert_stderr_contains("running `cargo check` on member1")
         .assert_stderr_not_contains("running `cargo check` on member2")
-        .assert_stderr_not_contains("skipped running on member1")
-        .assert_stderr_contains("skipped running on member2");
+        .assert_stderr_not_contains("skipped running on private crate member1")
+        .assert_stderr_contains("skipped running on private crate member2");
 }
 
 #[test]