Conversation
Add pager functionality to the following commands: - gist list - pr checks - release list - run list - run view - secret list - workflow list - workflow view Additionally, normalize error handling when starting the pager has failed: only print a non-fatal notice to stderr instead of aborting the whole command.
samcoe
left a comment
There was a problem hiding this comment.
Code looks good. Thanks for making this behavior consistent! I left a couple non-blocking comments.
| } | ||
| } | ||
|
|
||
| if err := opts.IO.StartPager(); err == nil { |
There was a problem hiding this comment.
How does a pager interpret/interact with the progress indicator? I think the pager was started in this command explicitly after the progress indicator, but I don't actually know if they conflict.
There was a problem hiding this comment.
Good question! In my testing it's fine if the progress indicator continues to spin even after the pager has started, but ideally it should never be started after the first output has been written to stdout.
| if s, ok := err.(api.HTTPError); ok && s.StatusCode == 404 { | ||
| if opts.Ref != "" { | ||
| return fmt.Errorf("could not find workflow file %s on %s, try specifying a different ref", workflow.Base(), opts.Ref) | ||
| if ref != "" { |
There was a problem hiding this comment.
Is there a reason to explicitly pass in ref here? Seems like we are still passing in opts.
There was a problem hiding this comment.
That's a valid observation, but I felt it was much more clear for the signature of the method to receive the ref as argument rather than inferring it from opts. Furthermore, I feel like having too many methods having opts as a receiver or argument is an antipattern because it dis-incentivizes the method from having a very limited responsibility. This small refactoring was a step towards not passing opts as an argument in the future.
Add pager functionality to the following commands:
Additionally, normalize error handling when starting the pager has failed: only print a non-fatal notice to stderr instead of aborting the whole command.
Fixes #5102