Skip to content

feat(pnpm): Add filter argument support#270

Open
KuSh wants to merge 4 commits intortk-ai:developfrom
KuSh:pnpm-filters
Open

feat(pnpm): Add filter argument support#270
KuSh wants to merge 4 commits intortk-ai:developfrom
KuSh:pnpm-filters

Conversation

@KuSh
Copy link

@KuSh KuSh commented Feb 27, 2026

  • Add support for pnpm global filter arguments (--filter) across all pnpm commands (list, outdated, install, etc.).
  • Merge pnpm filters with native command arguments (--filter + custom args like --depth).
  • Implement proper handling for Vec<String> (text filters) and Vec<OsString> (OS-specific filters).
  • Add unit tests for merge_pnpm_args and CLI parsing to validate expected behavior.

This enables users to apply workspace-specific filtering directly via --filter @scope before running pnpm commands, reducing token overhead by excluding irrelevant packages/dependencies from output.

Fixes: #259

@FlorianBruniaux
Copy link
Collaborator

Hi @KuSh! Quick ask — PR #241 just merged to master. Could you rebase this PR? One file change, should be straightforward. We'll review once it's current.

@KuSh
Copy link
Author

KuSh commented Mar 5, 2026

Hi @FlorianBruniaux, done!

Copy link
Contributor

@aeppling aeppling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(pnpm) Add filter argument support

Good addition overall — the approach is clean, additive, follows the existing Git global-args pattern, and all 623 tests pass with no new clippy warnings. A few things to address before merging:


Required

Build and Typecheck silently ignore --filter

When a user runs rtk pnpm --filter @app1 build, the filter is destructured but never used — no warning, no error. This will confuse users who expect filtering to apply.

At minimum, emit a warning when filters are provided with these subcommands:

if !filter.is_empty() {
    eprintln!("[rtk] warning: --filter is not yet supported for pnpm build, filters will be ignored");
}

Same for Typecheck. The FIXME comments acknowledge the gap but the silent discard is a UX issue that should not ship as-is.


Recommended (not blocking, but would strengthen the PR)

Missing test coverage for edge cases:

  • Empty filters: verify merge_pnpm_args(&[], &args) returns args unchanged (backward-compat)
  • Multiple --filter flags in CLI parsing: ["rtk", "pnpm", "--filter", "@app1", "--filter", "@app2", "list"] — only single filter is tested today
  • Other passthrough with filters: the OsString path is only tested at the helper level, not through CLI parsing → dispatch
  • Build/Typecheck with filters: document the expected behavior (filters ignored + warning)

Nits

  • The doc comment on merge_pnpm_args_os is a copy-paste of merge_pnpm_args — should mention it's the OsString variant
  • args.iter().map(|arg| arg.to_string()) in merge_pnpm_args could be .chain(args.iter().cloned()) — more idiomatic since String: Clone

Copy link
Contributor

@aeppling aeppling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing the PR we could merge this, user has informed with // FIXME and this is not blocking nor an real issue but a limitation.
Limitation is flagged and PR is valid.

@KuSh
Copy link
Author

KuSh commented Mar 6, 2026

I'll take review feedback into account, not too complicated and it helps develop skills

@KuSh
Copy link
Author

KuSh commented Mar 6, 2026

@aeppling your feedback is normally taken into account.

@pszymkowiak
Copy link
Collaborator

Rebased on develop, fixed two issues along the way:

  1. Added -F short form (#[arg(long, short = 'F')]) — pnpm accepts -F as alias for --filter
  2. Fixed validate_pnpm_filters return value that was silently discarded — now prints the warning via eprintln!
  3. pnpm build now routes through passthrough with filter merge (instead of next_cmd::run)

907 tests pass. @adrimusic could you review this one?

@pszymkowiak pszymkowiak requested a review from aeppling March 12, 2026 20:11
@KuSh KuSh marked this pull request as ready for review March 12, 2026 22:10
@KuSh
Copy link
Author

KuSh commented Mar 12, 2026

Thanks for lending a helping hand @pszymkowiak. I've made some fixes and additions:

  • Now that PnpmCommand::Build is simply another passthrough I've removed it completely
  • I've also removed the eprintln! call in validate_pnpm_filters to avoid double printed warning
  • There were some files added inadvertently, I've removed them
  • A git test was failing on my linux computer. I've fixed it (cmd.get_program() is returning /usr/bin/git instead of just git)

@KuSh KuSh force-pushed the pnpm-filters branch 3 times, most recently from 1c36e2b to d801f56 Compare March 12, 2026 23:34
@CLAassistant
Copy link

CLAassistant commented Mar 20, 2026

CLA assistant check
All committers have signed the CLA.

KuSh added 4 commits March 23, 2026 22:07
- Add support for pnpm global filter arguments (`--filter`) across all pnpm commands (`list`, `outdated`, `install`, etc.).
- Merge pnpm filters with native command arguments (`--filter` + custom args like `--depth`).
- Implement proper handling for `Vec<String>` (text filters) and `Vec<OsString>` (OS-specific filters).
- Add unit tests for `merge_pnpm_args` and CLI parsing to validate expected behavior.

This enables users to apply workspace-specific filtering directly via `--filter @scope` before running pnpm commands,
reducing token overhead by excluding irrelevant packages/dependencies from output.

Fixes: rtk-ai#259

FIXME: Add notes for typechecking/building with workspaces when filters are applied.
- Add validate_pnpm_filters() to warn when filters used with unsupported commands
- Add comprehensive tests for all scenarios
- Improve comments and use idiomatic rust
- Remove explicit PnpmCommands::Build variant (routed through Other)
- Simplify filter validation for Typecheck only
- Remove redundant warning eprintln!() call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pnpm --filter command results in rtk usage output

5 participants