Using default pager in gh pr diff #1534
Conversation
mmontes11
force-pushed
the
pr-diff-use-default-pager
branch
from
29cf9ae
to
a75dde0
Compare
pkg/cmd/pr/diff/diff.go
Outdated
| @@ -134,3 +142,26 @@ func isRemovalLine(dl string) bool { | |||
| func validColorFlag(c string) bool { | |||
| return c == "auto" || c == "always" || c == "never" | |||
| } | |||
|
|
|||
| func getDefaultPager() (string, error) { | |||
| getPagerCmd := git.GitCommand("config", "--get", "core.pager") | |||
There was a problem hiding this comment.
Respecting git's core.pager is something that I feel warrants discussion. On one hand, this might feel like the right thing to do in gh pr diff since it feels so much like git diff. On the other hand, at no point do we actually invoke git diff, so I don't feel strongly that we should support any of git's configuration around diffing and paging. In fact, instead of respecting git config core.pager or GIT_PAGER, I think we should only respect the PAGER environment variable.
@vilmibm Thoughts?
There was a problem hiding this comment.
I agree with @mislav that using core.pager for this can be weird. Also, using PAGER env variable makes testability of this feature easier, since we can mock that variable in tests!
mmontes11
force-pushed
the
pr-diff-use-default-pager
branch
3 times, most recently
from
b75e70a
to
2ca3e24
Compare
mmontes11
force-pushed
the
pr-diff-use-default-pager
branch
from
2ca3e24
to
df66a8f
Compare
There was a problem hiding this comment.
Looks good to me! BTW, when going through rounds of review-and-update with someone, it's helpful for the person reviewing (me) if you refrain from force-pushing so I can use the diff features to see only the changes from my last review. If there are too many messy commits in the branch, you can rebase it once we're all done with reviewing, or we will squash the whole thing when we merge 👍
Curious what @vilmibm has to say
| os.Setenv("PAGER", pager) | ||
| http.Verify(t) | ||
| }() | ||
| runPager = func(pager string, diff io.Reader, out io.Writer) error { |
There was a problem hiding this comment.
Note for @vilmibm: instead of overriding runPager here, maybe we could let it shell out to a real binary which is the go test binary, similar to what you've done for stubbing out shelling out to commands in the past? I feel like it would be an ideal case to bring back that approach
There was a problem hiding this comment.
I took that out because it was a difficult to debug hack...I think this is sufficient coverage for now. I'm guessing your concern is from the package level stubbing?
I'd rather see us step back and design a way to have factories send in external program callers.
|
My fault @mislav, next time will do the commit surgery after the rounds of review-and-update so you have a clear history. Thanks for the detailed feedback 🙏 |
|
thank you! |
Closes #1100
Changes:
PAGERenv variableSteps for testing this PR:
export PAGER='less -R';gh pr diff <pr-id>