Reject --comments with --json - #14215
Conversation
There was a problem hiding this comment.
Pull request overview
Rejects incompatible --comments and --json view-mode flags through shared JSON validation.
Changes:
- Adds validation for the conflicting flags.
- Adds unit coverage for the validation error.
Show a summary per file
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
67b3e0d to
2f6ec39
Compare
babakks
left a comment
babakks
left a comment
There was a problem hiding this comment.
Approved, but I'd rather do this check in the command checks rather than here.
@babakks I'll hold off on merging this because I want to check in on how strongly you feel on that - I wasn't sure on the direction here so I opted to just do them where the other mutual exclusion checks for |
|
I feel fairly strongly that the JSON flag handling shouldn't know about other arbitrary flags. Some of the ones in the same design space I understand (e.g. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2f6ec39 to
a18c1bc
Compare
|
@williammartin @babakks this has been rewritten |
Fixes #14214
Description
Passing
--commentswith--jsoncurrently succeeds, but JSON field selection takes precedence and silently ignores--comments.Reject this combination in the issue, pull request, and discussion view commands. Each command now returns "specify only one of --comments or --json" instead.
How did you test this change?
Given a built
bin/ghand an issueWhen I run
./bin/gh issue view 14214 --repo cli/cli --comments --json numberThen the command exits with "specify only one of --comments or --json" before making an API request.
Given the string-valued search qualifier
When I run
./bin/gh search issues --repo cli/cli --comments 5 --json number --limit 1and the equivalentsearch prscommandThen both commands return JSON results.
Key points
Each view command validates the modes it owns. Shared JSON flag handling remains unaware of
--comments, leaving string-valued search qualifiers compatible with JSON output.Notes for reviewers
Start with the three
NewCmdViewchecks and their constructor tests. The expected behavior was confirmed in #14214 (comment).Authorship and follow-up
Who wrote this:
Who answers review comments: