Conversation
|
thank you for taking the time to do this 🙏 |
mislav
left a comment
There was a problem hiding this comment.
Love how much cleanup this results in 👏
I've got one note about reading the repo info from the URL: it looks like we only respect the full URL in pr checkout but not in other pr commands. Now that there is a unified interface, I see it as a good opportunity to unify how URLs are handled across all commands.
api/queries_pr.go
Outdated
| func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, pr *PullRequest) (string, error) { | ||
| url := fmt.Sprintf("https://api.github.com/repos/%s/pulls/%d", | ||
| ghrepo.FullName(baseRepo), prNum) | ||
| ghrepo.FullName(baseRepo), pr.Number) |
There was a problem hiding this comment.
Was there a strong reason to change the call signature of this method? The way I see it, accepting an int instead of a PullRequest makes it more versatile (knowing the number, the caller may retrieve PR diff without first having to query the PR data itself), and if the caller already has a PullRequest reference they can easily pass the pr.Number as argument themeselves
There was a problem hiding this comment.
@vilmibm and I talked about this at one point. I think we thought it was more flexible to pass in the PR, but now that you mention that it sounds like it is LESS flexible now. I'll revert the change.
command/pr_checkout.go
Outdated
| r := regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) | ||
| if m := r.FindStringSubmatch(prString); m != nil { | ||
| prString = m[3] | ||
| baseRepo = ghrepo.New(m[1], m[2]) |
There was a problem hiding this comment.
This respects the base repo as found in the URL. 👍 I see this as currently only scoped to pr checkout, but could we afford to extend this to all pr commands via pr_lookup.go?
| r := regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) | ||
| if m := r.FindStringSubmatch(s); m != nil { | ||
| prNumberString := m[3] |
There was a problem hiding this comment.
It looks like this ignores the information about the base repo from the URL. To address issues such as #1060, would we consider respecting it?
|
@mislav check out 4c75c8b. |
mislav
left a comment
There was a problem hiding this comment.
Love it! Thanks for your patience and hard work.
I've added one more note about how we could avoid a superfluous API request
We've got a few ways to get a PR object right now, and they are all slightly different and require a lot of boilerplate code. This creates a consistent interface for that lookup. It might sound familiar because I tried it before in #926 but this time it will work!
The plan
This is going to be a two step process.
Get the commands using the same pr lookup interface
prFromArgs. This will look at the commands args to find the PR via a number, url, branch name or whatever the current branch of the existing git repo is. This is what I solved in this PR.Add an argument to
prFromArgsthat lets you pick what type of PR response you want back. This was mislav's magical reflection idea that we got working in https://github.com/cli/cli/compare/pr-get-queryThings I considered while writing this.
prFromArgsbecause if passed the string itself likeargs[0]we'd get a panic when there are no args or we'd need a conditional before each call. This seemed annoying and passing the args array worked out well enough.