feat(cli): make packages argument required for add/remove commands#277
Merged
feat(cli): make packages argument required for add/remove commands#277
Conversation
fengmk2
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors validation of the add and remove commands by making the packages argument required at the CLI parsing level instead of checking for empty packages within the command execution logic. This shifts the validation responsibility to the argument parser (clap), which provides better error messages and standardized behavior.
- Moved package validation from runtime to CLI argument parsing by adding
#[arg(required = true)] - Removed custom
NoPackagesSpecifiederror and related validation code - Updated snapshot tests to verify the new error behavior
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rfcs/add-remove-package-commands.md | Removed empty packages check from AddCommand and RemoveCommand execution methods |
| packages/cli/binding/src/commands/add.rs | Removed runtime validation and test for empty packages in AddCommand |
| packages/cli/binding/src/commands/remove.rs | Removed runtime validation and test for empty packages in RemoveCommand |
| packages/cli/binding/src/cli.rs | Added required = true attribute to packages arguments and removed test for empty packages |
| crates/vite_error/src/lib.rs | Removed NoPackagesSpecified error variant |
| packages/global/snap-tests/command-add-pnpm10/steps.json | Added test case for empty packages error and uncommented previously disabled tests |
| packages/global/snap-tests/command-add-pnpm10/snap.txt | Updated snapshot with new CLI help output and error messages showing required argument |
| packages/global/snap-tests/command-remove-pnpm10/steps.json | Added test case for empty packages error |
| packages/global/snap-tests/command-remove-pnpm10/snap.txt | Updated snapshot with new CLI help output and error messages showing required argument |
| packages/global/snap-tests/command-add-yarn4/snap.txt | Updated CLI help output to show required packages argument |
| packages/global/snap-tests/command-add-pnpm9/snap.txt | Updated CLI help output to show required packages argument |
| packages/global/snap-tests/command-add-npm11/snap.txt | Updated CLI help output to show required packages argument |
| packages/global/snap-tests/command-add-npm10/snap.txt | Updated CLI help output to show required packages argument |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
branchseer
approved these changes
Oct 30, 2025
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Make package arguments required for
addandremovecommands in the CLI.What changed?
required = trueattribute to thepackagesargument in bothaddandremovecommandsNoPackagesSpecifiederror from the error enum as it's no longer neededaddandremovecommand implementations<PACKAGES>...instead of[PACKAGES]...)How to test?
vp addwithout any packages and verify it shows an error about missing required argumentsvp removewithout any packages and verify it shows an error about missing required argumentsWhy make this change?
This change improves the CLI interface by making it clearer that package names are required for both
addandremovecommands. By using Clap's built-in argument validation instead of custom error handling, we get more consistent error messages and better help text. This also simplifies the code by removing unnecessary validation logic and error types.