feat: take review into account

- Add validate_pnpm_filters() to warn when filters used with unsupported commands
- Add comprehensive tests for all scenarios
- Improve comments and use idiomatic rust

Author: KuSh

src/main.rs

@@ -1186,24 +1186,49 @@ fn shell_split(input: &str) -> Vec<String> {
     tokens
 }
 
-/// Merge pnpm global filters args with other ones
+/// Merge pnpm global filters args with other ones for standard String-based commands
 fn merge_pnpm_args(filters: &[String], args: &[String]) -> Vec<String> {
     filters
         .iter()
         .map(|filter| format!("--filter={}", filter))
-        .chain(args.iter().map(|arg| arg.to_string()))
+        .chain(args.iter().cloned())
         .collect()
 }
 
-/// Merge pnpm global filters args with other ones
+/// Merge pnpm global filters args with other ones, using OsString for passthrough compatibility
 fn merge_pnpm_args_os(filters: &[String], args: &[OsString]) -> Vec<OsString> {
     filters
         .iter()
         .map(|filter| OsString::from(format!("--filter={}", filter)))
-        .chain(args.iter().map(|arg| arg.to_os_string()))
+        .chain(args.iter().cloned())
         .collect()
 }
 
+/// Validate that pnpm filters are only used in the global context, not before subcommands like build or tsc.
+fn validate_pnpm_filters(filters: &[String], command: &PnpmCommands) -> Option<String> {
+    // Check if this is a Build or Typecheck command with filters
+    match command {
+        PnpmCommands::Build { .. } | PnpmCommands::Typecheck { .. } => {
+            // FIXME: if filters are present, we should find out which workspaces are build before running build
+            if !filters.is_empty() {
+                let cmd_name = match command {
+                    PnpmCommands::Build { .. } => "build",
+                    PnpmCommands::Typecheck { .. } => "tsc",
+                    _ => unreachable!(),
+                };
+                let msg = format!(
+                    "[rtk] warning: --filter is not yet supported for pnpm {}, filters preceding the subcommand will be ignored",
+                    cmd_name
+                );
+                eprintln!("{}", msg);
+                return Some(msg);
+            }
+            None
+        }
+        _ => None,
+    }
+}
+
 fn main() -> Result<()> {
     // Fire-and-forget telemetry ping (1/day, non-blocking)
     telemetry::maybe_ping();
@@ -1426,42 +1451,49 @@ fn main() -> Result<()> {
             psql_cmd::run(&args, cli.verbose)?;
         }
 
-        Commands::Pnpm { filter, command } => match command {
-            PnpmCommands::List { depth, args } => {
-                pnpm_cmd::run(
-                    pnpm_cmd::PnpmCommand::List { depth },
-                    &merge_pnpm_args(&filter, &args),
-                    cli.verbose,
-                )?;
-            }
-            PnpmCommands::Outdated { args } => {
-                pnpm_cmd::run(
-                    pnpm_cmd::PnpmCommand::Outdated,
-                    &merge_pnpm_args(&filter, &args),
-                    cli.verbose,
-                )?;
+        Commands::Pnpm { filter, command } => {
+            // Warns user if filters are used with unsupported subcommands like build or typecheck
+            if let Some(warning) = validate_pnpm_filters(&filter, &command) {
+                eprintln!("{}", warning);
             }
-            PnpmCommands::Install { packages, args } => {
-                pnpm_cmd::run(
-                    pnpm_cmd::PnpmCommand::Install { packages },
-                    &merge_pnpm_args(&filter, &args),
-                    cli.verbose,
-                )?;
-            }
-            PnpmCommands::Build { args } => {
-                let mut build_args: Vec<String> = vec!["build".into()];
-                build_args.extend(args);
-                let os_args: Vec<OsString> = build_args.into_iter().map(OsString::from).collect();
-                pnpm_cmd::run_passthrough(&merge_pnpm_args_os(&filter, &os_args), cli.verbose)?;
-            }
-            PnpmCommands::Typecheck { args } => {
-                // FIXME: if filters are present, we should find out which workspaces are typechecked before running tsc
-                tsc_cmd::run(&args, cli.verbose)?;
-            }
-            PnpmCommands::Other(args) => {
-                pnpm_cmd::run_passthrough(&merge_pnpm_args_os(&filter, &args), cli.verbose)?;
+
+            match command {
+                PnpmCommands::List { depth, args } => {
+                    pnpm_cmd::run(
+                        pnpm_cmd::PnpmCommand::List { depth },
+                        &merge_pnpm_args(&filter, &args),
+                        cli.verbose,
+                    )?;
+                }
+                PnpmCommands::Outdated { args } => {
+                    pnpm_cmd::run(
+                        pnpm_cmd::PnpmCommand::Outdated,
+                        &merge_pnpm_args(&filter, &args),
+                        cli.verbose,
+                    )?;
+                }
+                PnpmCommands::Install { packages, args } => {
+                    pnpm_cmd::run(
+                        pnpm_cmd::PnpmCommand::Install { packages },
+                        &merge_pnpm_args(&filter, &args),
+                        cli.verbose,
+                    )?;
+                }
+                PnpmCommands::Build { args } => {
+                    let mut build_args: Vec<String> = vec!["build".into()];
+                    build_args.extend(args);
+                    let os_args: Vec<OsString> =
+                        build_args.into_iter().map(OsString::from).collect();
+                    pnpm_cmd::run_passthrough(&merge_pnpm_args_os(&filter, &os_args), cli.verbose)?;
+                }
+                PnpmCommands::Typecheck { args } => {
+                    tsc_cmd::run(&args, cli.verbose)?;
+                }
+                PnpmCommands::Other(args) => {
+                    pnpm_cmd::run_passthrough(&merge_pnpm_args_os(&filter, &args), cli.verbose)?;
+                }
             }
-        },
+        }
 
         Commands::Err { command } => {
             let cmd = command.join(" ");
@@ -2511,6 +2543,14 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_merge_filters_with_no_args() {
+        let filters = vec![];
+        let args = vec!["--depth=0".to_string(), "--no-verbose".to_string()];
+        let expected_args = vec!["--depth=0", "--no-verbose"];
+        assert_eq!(merge_pnpm_args(&filters, &args), expected_args);
+    }
+
     #[test]
     fn test_merge_filters_with_args() {
         let filters = vec!["@app1".to_string(), "@app2".to_string()];
@@ -2529,6 +2569,14 @@ mod tests {
         assert_eq!(merge_pnpm_args(&filters, &args), expected_args);
     }
 
+    #[test]
+    fn test_merge_filters_with_no_args_os() {
+        let filters = vec![];
+        let args = vec![OsString::from("--depth=0")];
+        let expected_args = vec![OsString::from("--depth=0")];
+        assert_eq!(merge_pnpm_args_os(&filters, &args), expected_args);
+    }
+
     #[test]
     fn test_merge_filters_with_args_os() {
         let filters = vec!["@app1".to_string()];
@@ -2542,15 +2590,22 @@ mod tests {
 
     #[test]
     fn test_pnpm_subcommand_with_filter() {
-        let cli = Cli::try_parse_from(["rtk", "pnpm", "--filter", "@app1", "list"]).unwrap();
+        let cli = Cli::try_parse_from([
+            "rtk", "pnpm", "--filter", "@app1", "--filter", "@app2", "list", "--filter", "@app3",
+            "--filter", "@app4", "--prod",
+        ])
+        .unwrap();
         match cli.command {
             Commands::Pnpm {
                 filter,
                 command: PnpmCommands::List { depth, args },
             } => {
                 assert_eq!(depth, 0);
-                assert_eq!(filter, vec!["@app1"]);
-                assert!(args.is_empty());
+                assert_eq!(filter, vec!["@app1", "@app2"]);
+                assert_eq!(
+                    args,
+                    vec!["--filter", "@app3", "--filter", "@app4", "--prod"]
+                );
             }
             _ => panic!("Expected Pnpm List command"),
         }
@@ -2568,4 +2623,89 @@ mod tests {
             _ => panic!("Expected Pnpm command"),
         }
     }
+
+    #[test]
+    fn test_pnpm_build_without_filters() {
+        let cli = Cli::try_parse_from([
+            "rtk", "pnpm", "build", "--filter", "@app3", "--filter", "@app4",
+        ])
+        .unwrap();
+        match cli.command {
+            Commands::Pnpm { filter, command } => {
+                let warning = validate_pnpm_filters(&filter, &command);
+
+                assert!(filter.is_empty());
+                assert!(warning.is_none())
+            }
+            _ => panic!("Expected Pnpm Build command"),
+        }
+    }
+
+    #[test]
+    fn test_pnpm_build_with_filters() {
+        let cli = Cli::try_parse_from([
+            "rtk", "pnpm", "--filter", "@app1", "--filter", "@app2", "build", "--filter", "@app3",
+            "--filter", "@app4",
+        ])
+        .unwrap();
+        match cli.command {
+            Commands::Pnpm { filter, command } => {
+                let warning = validate_pnpm_filters(&filter, &command).unwrap();
+
+                assert_eq!(filter, vec!["@app1", "@app2"]);
+                assert_eq!(warning, "[rtk] warning: --filter is not yet supported for pnpm build, filters preceding the subcommand will be ignored")
+            }
+            _ => panic!("Expected Pnpm Build command"),
+        }
+    }
+
+    #[test]
+    fn test_pnpm_typecheck_without_filters() {
+        let cli = Cli::try_parse_from([
+            "rtk",
+            "pnpm",
+            "typecheck",
+            "--filter",
+            "@app3",
+            "--filter",
+            "@app4",
+        ])
+        .unwrap();
+        match cli.command {
+            Commands::Pnpm { filter, command } => {
+                let warning = validate_pnpm_filters(&filter, &command);
+
+                assert!(filter.is_empty());
+                assert!(warning.is_none())
+            }
+            _ => panic!("Expected Pnpm Build command"),
+        }
+    }
+
+    #[test]
+    fn test_pnpm_typecheck_with_filters() {
+        let cli = Cli::try_parse_from([
+            "rtk",
+            "pnpm",
+            "--filter",
+            "@app1",
+            "--filter",
+            "@app2",
+            "typecheck",
+            "--filter",
+            "@app3",
+            "--filter",
+            "@app4",
+        ])
+        .unwrap();
+        match cli.command {
+            Commands::Pnpm { filter, command } => {
+                let warning = validate_pnpm_filters(&filter, &command).unwrap();
+
+                assert_eq!(filter, vec!["@app1", "@app2"]);
+                assert_eq!(warning, "[rtk] warning: --filter is not yet supported for pnpm tsc, filters preceding the subcommand will be ignored")
+            }
+            _ => panic!("Expected Pnpm Build command"),
+        }
+    }
 }