Conversation
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Should we consider existing with a nonzero status to indicate that the command "failed" (i.e. did not found any matching items)? I'm 50-50 on this one, and we can defer this decision until another time.
command/pr.go
Outdated
|
|
||
| if len(prs) == 0 { | ||
| colorOut := colorableOut(cmd) | ||
| printMessage(colorOut, "There are no open pull requests") |
There was a problem hiding this comment.
A couple of notes (more like a note avalanche):
-
I think we should treat this message as a warning, and print it to stderr instead to stdout. This is because somebody might be piping
pr listto a script, and they definitely don't want to process this message as if it were a PR entry. -
This message will be wrong if somebody invoked
pr list --state closed. In this case, we should print “There are no closed pull requests”. Reword descriptive text if all issues are open #134 -
How about indicating other filtering options? E.g. when
--assigneewas supplied, we might want to say “There are no open pull requests assigned to X”, or an equivalent for labels: Improve experience when no matches found forpr/issue list --label#89.On the other hand, generating English sentences for each permutation of filtering options can be a slippery slope towards code that's hard to maintain. Maybe we can circumvent all that just by always printing “There are no matching pull requests”.
There was a problem hiding this comment.
Here's what I'm leaning to, for simplicity:
- No flags passed: “There are no open pull requests”
- Some flags passed: “No pull requests matched your search” (to mirror “No results matched your search” shown by dotcom in the same scenario)
There was a problem hiding this comment.
Piping this to stderr makes a lot of sense to me, also your suggestion of limiting the text to two messages is a good compromise. I'm going to go that direction.
|
All right, after struggling with getting the flag values out of Cobra, this is what I've got. All that output is sent to stderr as well. |
| msg := "There are no open issues" | ||
|
|
||
| userSetFlags := false | ||
| cmd.Flags().VisitAll(func(f *pflag.Flag) { |
There was a problem hiding this comment.
Tip: if you use Visit instead of VisitAll, it only visits "changed" flags
|
|
||
| userSetFlags := false | ||
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | ||
| userSetFlags = f.Changed || userSetFlags |
There was a problem hiding this comment.
This is going to trigger on any flag that has a value set, including ones that don't change filtering options, such as -L 10 or -R owner/repo.
Perhaps we could put the names of filtering-related flags in a list and make sure we only iterate over those and no others?
There was a problem hiding this comment.
For the user the difference will be pretty small ("No pull requests match your search" vs "There are no open pull requests") and I think showing the "match your search" text still makes sense when the user supplies arguments. And adding that code is a little fragile because there would be no feedback when we add new commands, so I'm hesitant about relying on a list.
ghcs delete: deprecate argument/subcommands & introduce flags
3 participants
Where there are no PRs the output now looks like this...
Closes #134
Closes #138