Return empty error before starting a pager program #6419
Conversation
There was a problem hiding this comment.
Thank you! I've left a bunch of notes, mostly the same gotcha regarding checking for the presence of the exporter.
Since this change repeats similar logic across several different commands, I wonder whether we could somehow capture it from a central location, making this automatically handled even if the command didn't necessarily add these conditionals?
For example, opts.IO.StartPager() could just queue a pager for starting, but not actually start it. Then, we could replace opts.IO.Out with a Writer that, on first write, automatically starts the pager. That way, the pager will never be started for empty output, and individual commands don't have to worry about this. Just a wild idea (you don't have to actually implement this in your PR)
pkg/cmd/pr/checks/checks.go
Outdated
| return err | ||
| } | ||
|
|
||
| checks, counts, err = aggregateChecks(pr, opts.Required) |
There was a problem hiding this comment.
Since we now do the combo of populateStatusChecks + aggregateChecks twice, would it make sense to capture both actions as a single helper function?
There was a problem hiding this comment.
I moved aggregateChecks inside of populateStatusChecks. I think it makes more sense that populateStatusChecks returns usable values than just an error.
|
Thank you for your feedback @mislav. Fixed as you requested. When I looked at this issue I think I could fix |
mislav
left a comment
There was a problem hiding this comment.
Looks great! One tiny nitpick
pkg/cmd/pr/checks/checks.go
Outdated
|
|
||
| if len(resp.Node.StatusCheckRollup.Nodes) == 0 { | ||
| return nil | ||
| return aggregateChecks(pr, requiredChecks) |
There was a problem hiding this comment.
Style tip: let's instead break here and allow the final return from the parent function to get reached.
There was a problem hiding this comment.
Fixed!
I was thinking break might change the logic because there're some statements before the final return. But I don't fully understand the whole code in this file.
There was a problem hiding this comment.
@mislav I quickly pushed last night without running tests. It turned out that break here did break tests. I reverted the commit for now.
mislav
left a comment
There was a problem hiding this comment.
I also didn't fully understand what was going on in the populateStatusChecks function, but I had a hunch that some of it was unnecessary, so I've made a pass to simplify.
Fixes #6400
Return empty error before starting a pager program.
Previously, these commands start
pagereven with empty results:For example, the output with
PAGER=lessshowsAn error is printed after quitting a pager program.