STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #11192 from cli/babakks/decouple-arg-parsing-from-pr-finder

Browse source 
Decouple arg parsing from PR finder
This commit is contained in:
Babak K. Shandiz 2025-07-03 11:02:55 +01:00 • committed by GitHub
commit 3680a1fb22
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 156 additions and 156 deletions
Download patch file Download diff file Expand all files Collapse all files

28
api/queries_pr.go
View file

@ -85,25 +85,15 @@ type PullRequest struct {
Assignees Assignees
AssignedActors AssignedActors
// AssignedActorsUsed is a GIGANTIC hack to carry around whether we expected AssignedActors to be requested
// on this PR. This is required because the Feature Detection of support for AssignedActors occurs inside the
// PR Finder, but knowledge of support is required at the command level. However, we can't easily construct
// the feature detector at the command level because it needs knowledge of the BaseRepo, which is only available
// inside the PR Finder. This is bad and we should feel bad.
//
// The right solution is to extract argument parsing from the PR Finder into each command, so that we have access
// to the BaseRepo and can construct the feature detector there. This is what happens in the issue commands with
// `shared.ParseIssueFromArg`.
AssignedActorsUsed bool
Labels Labels
ProjectCards ProjectCards
ProjectItems ProjectItems
Milestone *Milestone
Comments Comments
ReactionGroups ReactionGroups
Reviews PullRequestReviews
LatestReviews PullRequestReviews
ReviewRequests ReviewRequests
Labels Labels
ProjectCards ProjectCards
ProjectItems ProjectItems
Milestone *Milestone
Comments Comments
ReactionGroups ReactionGroups
Reviews PullRequestReviews
LatestReviews PullRequestReviews
ReviewRequests ReviewRequests
ClosingIssuesReferences ClosingIssuesReferences
}

4
pkg/cmd/issue/edit/edit_test.go
View file

@ -263,12 +263,12 @@ func TestNewCmdEdit(t *testing.T) {
},
{
name: "argument is a URL",
input: "https://github.com/cli/cli/issues/23",
input: "https://example.com/cli/cli/issues/23",
output: EditOptions{
IssueNumbers: []int{23},
Interactive: true,
},
expectedBaseRepo: ghrepo.New("cli", "cli"),
expectedBaseRepo: ghrepo.NewWithHost("cli", "cli", "example.com"),
wantsErr: false,
},
{

52
pkg/cmd/pr/edit/edit.go
View file

@ -3,6 +3,7 @@ package edit
import (
"fmt"
"net/http"
"time"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/v2/api"
@ -81,12 +82,28 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
opts.Finder = shared.NewFinder(f)
// support `-R, --repo` override
opts.BaseRepo = f.BaseRepo
if len(args) > 0 {
opts.SelectorArg = args[0]
}
if opts.SelectorArg != "" {
// If a URL is provided, we need to parse it to override the
// base repository, especially the hostname part. That's because
// we need a feature detector down in this command, and that
// needs to know the API host. If the command is run outside of
// a git repo, we cannot instantiate the detector unless we have
// already parsed the URL.
if baseRepo, _, err := shared.ParseURL(opts.SelectorArg); err == nil {
opts.BaseRepo = func() (ghrepo.Interface, error) {
return baseRepo, nil
}
}
}
flags := cmd.Flags()
bodyProvided := flags.Changed("body")
@ -203,17 +220,38 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
}
func editRun(opts *EditOptions) error {
findOptions := shared.FindOptions{
Selector: opts.SelectorArg,
Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone", "assignees"},
Detector: opts.Detector,
}
httpClient, err := opts.HttpClient()
if err != nil {
return err
}
if opts.Detector == nil {
baseRepo, err := opts.BaseRepo()
if err != nil {
return err
}
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
}
findOptions := shared.FindOptions{
Selector: opts.SelectorArg,
Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"},
Detector: opts.Detector,
}
issueFeatures, err := opts.Detector.IssueFeatures()
if err != nil {
return err
}
if issueFeatures.ActorIsAssignable {
findOptions.Fields = append(findOptions.Fields, "assignedActors")
} else {
findOptions.Fields = append(findOptions.Fields, "assignees")
}
pr, repo, err := opts.Finder.Find(findOptions)
if err != nil {
return err
@ -225,7 +263,7 @@ func editRun(opts *EditOptions) error {
editable.Body.Default = pr.Body
editable.Base.Default = pr.BaseRefName
editable.Reviewers.Default = pr.ReviewRequests.Logins()
if pr.AssignedActorsUsed {
if issueFeatures.ActorIsAssignable {
editable.Assignees.ActorAssignees = true
editable.Assignees.Default = pr.AssignedActors.DisplayNames()
editable.Assignees.DefaultLogins = pr.AssignedActors.Logins()

51
pkg/cmd/pr/edit/edit_test.go
View file

@ -26,11 +26,12 @@ func TestNewCmdEdit(t *testing.T) {
require.NoError(t, err)
tests := []struct {
name string
input string
stdin string
output EditOptions
wantsErr bool
name string
input string
stdin string
output EditOptions
expectedBaseRepo ghrepo.Interface
wantsErr bool
}{
{
name: "no argument",
@ -47,6 +48,16 @@ func TestNewCmdEdit(t *testing.T) {
output: EditOptions{},
wantsErr: true,
},
{
name: "URL argument",
input: "https://example.com/cli/cli/pull/23",
output: EditOptions{
SelectorArg: "https://example.com/cli/cli/pull/23",
Interactive: true,
},
expectedBaseRepo: ghrepo.NewWithHost("cli", "cli", "example.com"),
wantsErr: false,
},
{
name: "pull request number argument",
input: "23",
@ -326,6 +337,15 @@ func TestNewCmdEdit(t *testing.T) {
assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg)
assert.Equal(t, tt.output.Interactive, gotOpts.Interactive)
assert.Equal(t, tt.output.Editable, gotOpts.Editable)
if tt.expectedBaseRepo != nil {
baseRepo, err := gotOpts.BaseRepo()
require.NoError(t, err)
require.True(
t,
ghrepo.IsSame(tt.expectedBaseRepo, baseRepo),
"expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo,
)
}
})
}
}
@ -344,8 +364,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: false,
Editable: shared.Editable{
@ -408,8 +427,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: false,
Editable: shared.Editable{
@ -466,8 +484,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: false,
Editable: shared.Editable{
@ -529,8 +546,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: true,
Surveyor: testSurveyor{
@ -576,8 +592,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: true,
Surveyor: testSurveyor{
@ -620,8 +635,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
}, ghrepo.New("OWNER", "REPO")),
Interactive: true,
Surveyor: testSurveyor{
@ -667,8 +681,7 @@ func Test_editRun(t *testing.T) {
Detector: &fd.EnabledDetectorMock{},
SelectorArg: "123",
Finder: shared.NewMockFinder("123", &api.PullRequest{
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActorsUsed: true,
URL: "https://github.com/OWNER/REPO/pull/123",
AssignedActors: api.AssignedActors{
Nodes: []api.Actor{
{

37
pkg/cmd/pr/shared/finder.go
View file

@ -112,7 +112,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
return nil, nil, errors.New("Find error: no fields specified")
}
if repo, prNumber, err := f.parseURL(opts.Selector); err == nil {
if repo, prNumber, err := ParseURL(opts.Selector); err == nil {
f.prNumber = prNumber
f.baseRefRepo = repo
}
@ -242,33 +242,6 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
}
}
// Ok this is super, super horrible so bear with me.
// The `assignees` field on a Pull Request exposes users that are assigned. It is also possible for bots to be
// assigned, but they only appear under the `assignedActors` field. Ideally, the caller of `Find` would determine
// the correct field to use based on the `fd.Detector` that is passed in, but they can't construct a detector
// because the BaseRepo is only determined within this function. The more correct solution is to do what I did with
// the issue commands and decouple argument parsing from API lookup. See PR #10811 for example.
var actorAssigneesUsed bool
if fields.Contains("assignees") {
if opts.Detector == nil {
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost())
}
issueFeatures, err := opts.Detector.IssueFeatures()
if err != nil {
return nil, nil, fmt.Errorf("error detecting issue features: %v", err)
}
// If actors are assignable on this host then we additionally request the `assignedActors` field.
// Note that we don't remove the `assignees` field because some commands (`pr view`) do not display actor
// assignees yet, so we have to have both sets of data.
if issueFeatures.ActorIsAssignable {
fields.Add("assignedActors")
actorAssigneesUsed = true
}
}
var pr *api.PullRequest
if f.prNumber > 0 {
// If we have a PR number, let's look it up
@ -324,16 +297,14 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
})
}
if actorAssigneesUsed {
pr.AssignedActorsUsed = true
}
return pr, f.baseRefRepo, g.Wait()
}
var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`)
func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) {
// ParseURL parses a pull request URL and returns the repository and pull
// request number.
func ParseURL(prURL string) (ghrepo.Interface, int, error) {
if prURL == "" {
return nil, 0, fmt.Errorf("invalid URL: %q", prURL)
}

140
pkg/cmd/pr/shared/finder_test.go
View file

@ -9,12 +9,75 @@ import (
ghContext "github.com/cli/cli/v2/context"
"github.com/cli/cli/v2/git"
fd "github.com/cli/cli/v2/internal/featuredetection"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/httpmock"
"github.com/stretchr/testify/require"
)
func TestParseURL(t *testing.T) {
tests := []struct {
name string
arg string
wantRepo ghrepo.Interface
wantNum int
wantErr string
}{
{
name: "valid HTTPS URL",
arg: "https://example.com/owner/repo/pull/123",
wantRepo: ghrepo.NewWithHost("owner", "repo", "example.com"),
wantNum: 123,
},
{
name: "valid HTTP URL",
arg: "http://example.com/owner/repo/pull/123",
wantRepo: ghrepo.NewWithHost("owner", "repo", "example.com"),
wantNum: 123,
},
{
name: "empty URL",
wantErr: "invalid URL: \"\"",
},
{
name: "invalid scheme",
arg: "ftp://github.com/owner/repo/pull/123",
wantErr: "invalid scheme: ftp",
},
{
name: "incorrect path",
arg: "https://github.com/owner/repo/issues/123",
wantErr: "not a pull request URL: https://github.com/owner/repo/issues/123",
},
{
name: "no PR number",
arg: "https://github.com/owner/repo/pull/",
wantErr: "not a pull request URL: https://github.com/owner/repo/pull/",
},
{
name: "invalid PR number",
arg: "https://github.com/owner/repo/pull/foo",
wantErr: "not a pull request URL: https://github.com/owner/repo/pull/foo",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
repo, num, err := ParseURL(tt.arg)
if tt.wantErr != "" {
require.Error(t, err)
require.Equal(t, tt.wantErr, err.Error())
return
}
require.NoError(t, err)
require.Equal(t, tt.wantNum, num)
require.NotNil(t, repo)
require.True(t, ghrepo.IsSame(tt.wantRepo, repo))
})
}
}
type args struct {
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
@ -706,81 +769,6 @@ func TestFind(t *testing.T) {
}
}
func TestFindAssignableActors(t *testing.T) {
t.Run("given actors are not assignable, do nothing special", func(t *testing.T) {
reg := &httpmock.Registry{}
defer reg.Verify(t)
// Ensure we never request assignedActors
reg.Exclude(t, httpmock.GraphQL(`assignedActors`))
reg.Register(
httpmock.GraphQL(`query PullRequestByNumber\b`),
httpmock.StringResponse(`{"data":{"repository":{
"pullRequest":{"number":13}
}}}`))
f := finder{
httpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
}
pr, _, err := f.Find(FindOptions{
Detector: &fd.DisabledDetectorMock{},
Fields: []string{"assignees"},
Selector: "https://github.com/cli/cli/pull/13",
})
require.NoError(t, err)
require.False(t, pr.AssignedActorsUsed, "expected PR not to have assigned actors used")
})
t.Run("given actors are assignable, request assignedActors and indicate that on the returned PR", func(t *testing.T) {
reg := &httpmock.Registry{}
defer reg.Verify(t)
// Ensure that we only respond if assignedActors is requested
reg.Register(
httpmock.GraphQL(`assignedActors`),
httpmock.StringResponse(`{"data":{"repository":{
"pullRequest":{
"number":13,
"assignedActors": {
"nodes": [
{
"id": "HUBOTID",
"login": "hubot",
"__typename": "Bot"
},
{
"id": "MONAID",
"login": "MonaLisa",
"name": "Mona Display Name",
"__typename": "User"
}
],
"totalCount": 2
}}
}}}`))
f := finder{
httpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
}
pr, _, err := f.Find(FindOptions{
Detector: &fd.EnabledDetectorMock{},
Fields: []string{"assignees"},
Selector: "https://github.com/cli/cli/pull/13",
})
require.NoError(t, err)
require.Equal(t, []string{"hubot", "MonaLisa"}, pr.AssignedActors.Logins())
require.True(t, pr.AssignedActorsUsed, "expected PR to have assigned actors used")
})
}
func stubBranchConfig(branchConfig git.BranchConfig, err error) func(context.Context, string) (git.BranchConfig, error) {
return func(_ context.Context, branch string) (git.BranchConfig, error) {
return branchConfig, err