cli(feat) Show help instead of errors when commands lack required args#511
Merged
cli(feat) Show help instead of errors when commands lack required args#511
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 80.63% 80.83% +0.20%
==========================================
Files 16 16
Lines 2236 2249 +13
Branches 466 471 +5
==========================================
+ Hits 1803 1818 +15
+ Misses 279 276 -3
- Partials 154 155 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tony
force-pushed
the
helpful-cli
branch
2 times, most recently
from
d7f76e8 to
15d7f0f
Compare
10 tasks
tony
force-pushed
the
helpful-cli
branch
2 times, most recently
from
0bc0c14 to
f7c0d21
Compare
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
tony
force-pushed
the
helpful-cli
branch
5 times, most recently
from
3488c8b to
95a2f31
Compare
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. Review scope: 5 parallel reviewers checked CLAUDE.md adherence, shallow bug scan, git history context, previous PR comments, and code comment compliance across all 8 modified files (207 additions, 67 deletions). One pre-existing note (not blocking): 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
why: Improve developer experience by providing helpful guidance instead of cryptic argparse errors or confusing behavior when CLI commands are called without required arguments. what: - Add --all/-a flag to sync command (BREAKING: sync now requires --all to sync all repos, previously synced none silently) - Show help for sync when no patterns and --all not specified - Show help for search when no query terms provided (changed nargs="+" to "*") - Show help for add when no repo_path provided (added nargs="?") - Show help for discover when no scan_dir provided (added nargs="?") - Update tests to verify new help-on-empty behavior - Update CLI examples to show vcspull sync --all
why: -a is the short form of --all but was absent from OPTIONS_FLAG_ONLY, causing the help colorizer to potentially misclassify the next token. what: - Add "-a" to the OPTIONS_FLAG_ONLY set
why: The "*" glob pattern is the old way to sync all repos. Help text should consistently recommend --all instead. what: - Replace 'vcspull sync --dry-run "*"' with 'vcspull sync --dry-run --all' - Replace 'vcspull sync -f ./myrepos.yaml "*"' with 'vcspull sync -f ./myrepos.yaml --all'
why: When --all is specified, positional patterns are silently ignored because the sync_all branch loads all configs and skips pattern matching. This is confusing UX — users think they are filtering. what: - Add log.warning() when both sync_all and repo_patterns are provided - Add test fixture for sync --all with a pattern
why: When sync() is called programmatically with no patterns, no --all, and no parser, it silently returns with no feedback. Programmatic callers have no way to know nothing happened. what: - Add log.warning() in the else branch when parser is None - Add test_sync_no_patterns_no_parser_warns test
why: SyncFixture now tests search, add, and discover subcommands too, making the name misleading. what: - Rename SyncFixture → CLIFixture - Rename SYNC_REPO_FIXTURES → CLI_FIXTURES - Rename test_sync → test_cli_subcommands
why: The combination of --all and --dry-run was untested despite exercising the async plan-building path for all configured repos. what: - Add CLIFixture entry for ["sync", "--all", "--dry-run"] - Assert "Plan:" appears in output
…terns why: Address multi-model review consensus finding (Claude+GPT) — combining --all with patterns silently ignored the patterns, risking unintended syncs of all repos when users meant to filter. what: - Replace log.warning with parser.error() when --all and patterns coexist - Exit with code 2 (standard argparse error) instead of proceeding - Update test to expect exit code 2 and error in stderr
why: Address multi-model review consensus finding (Claude+GPT) — the field name was misleading since CLIFixture now tests all subcommands, not just sync. what: - Rename sync_args field to cli_args in CLIFixture NamedTuple - Rename sync_args parameter to cli_args in test_cli_subcommands - Rename local cli_args variable to resolved_args to avoid shadowing
why: Address multi-model review finding (Claude) — no tests exercised the -a short alias, and user requested verifying --all equals "*" behavior. what: - Add sync--a-short-flag test confirming -a works like --all - Add sync--star-pattern test confirming "*" matches all repos like --all
why: Address multi-model review finding (Claude) — documentation referenced SyncFixture/SYNC_REPO_FIXTURES which were renamed to CLIFixture/CLI_FIXTURES. what: - Update NamedTuple example from SyncFixture to CLIFixture - Update field name from sync_args to cli_args - Update fixture list name from SYNC_REPO_FIXTURES to CLI_FIXTURES - Update test function name from test_sync to test_cli_subcommands
why: Document user-facing changes from the helpful-cli branch for the upcoming release. what: - Add "What's new" entry for help screens on empty CLI invocations - Add "Breaking changes" entry for sync --all requirement
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
1 participant
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.
Summary
All four CLI subcommands now print their help screen when called
without required arguments, instead of raising argparse errors or
silently doing nothing:
vcspull syncvcspull searchvcspull addvcspull discoverThis makes the CLI self-documenting — new users can explore
available options without reaching for
--help, and experiencedusers get an instant reminder of the expected syntax.
New:
--all/-aflag for syncvcspull sync --allsyncs every configured repository. The oldvcspull sync "*"glob still works, but--allis therecommended way going forward.
--alland positional patterns are mutually exclusive — combiningthem is an error.
Breaking change
vcspull sync(no arguments) previously synced nothing and exitedsilently. It now shows help. Use
vcspull sync --allto sync allrepositories.
Test plan
uv run pytest)vcspull sync→ shows helpvcspull sync --all→ syncs all reposvcspull sync -a→ syncs all repos (short flag)vcspull sync --all "django-*"→ error (mutually exclusive)vcspull sync "*"→ syncs all repos (backward compat)vcspull search→ shows helpvcspull add→ shows helpvcspull discover→ shows help