diff --git a/command/alias.go b/command/alias.go index 2a9e13fbc..a820268fe 100644 --- a/command/alias.go +++ b/command/alias.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/spf13/cobra" @@ -166,9 +167,9 @@ func aliasList(cmd *cobra.Command, args []string) error { return nil } - stdout := colorableOut(cmd) - - tp := utils.NewTablePrinter(stdout) + tp := utils.NewTablePrinter(&iostreams.IOStreams{ + Out: cmd.OutOrStdout(), + }) aliasMap := aliasCfg.All() keys := []string{} diff --git a/command/issue.go b/command/issue.go index 192ac593c..41dd30b7f 100644 --- a/command/issue.go +++ b/command/issue.go @@ -3,7 +3,6 @@ package command import ( "fmt" "io" - "net/url" "strconv" "strings" "time" @@ -15,6 +14,7 @@ import ( "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/githubtemplate" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/text" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -122,57 +122,6 @@ var issueReopenCmd = &cobra.Command{ RunE: issueReopen, } -type filterOptions struct { - entity string - state string - assignee string - labels []string - author string - baseBranch string - mention string - milestone string -} - -func listURLWithQuery(listURL string, options filterOptions) (string, error) { - u, err := url.Parse(listURL) - if err != nil { - return "", err - } - query := fmt.Sprintf("is:%s ", options.entity) - if options.state != "all" { - query += fmt.Sprintf("is:%s ", options.state) - } - if options.assignee != "" { - query += fmt.Sprintf("assignee:%s ", options.assignee) - } - for _, label := range options.labels { - query += fmt.Sprintf("label:%s ", quoteValueForQuery(label)) - } - if options.author != "" { - query += fmt.Sprintf("author:%s ", options.author) - } - if options.baseBranch != "" { - query += fmt.Sprintf("base:%s ", options.baseBranch) - } - if options.mention != "" { - query += fmt.Sprintf("mentions:%s ", options.mention) - } - if options.milestone != "" { - query += fmt.Sprintf("milestone:%s ", quoteValueForQuery(options.milestone)) - } - q := u.Query() - q.Set("q", strings.TrimSuffix(query, " ")) - u.RawQuery = q.Encode() - return u.String(), nil -} - -func quoteValueForQuery(v string) string { - if strings.ContainsAny(v, " \"\t\r\n") { - return fmt.Sprintf("%q", v) - } - return v -} - func issueList(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) @@ -230,14 +179,14 @@ func issueList(cmd *cobra.Command, args []string) error { if web { issueListURL := ghrepo.GenerateRepoURL(baseRepo, "issues") - openURL, err := listURLWithQuery(issueListURL, filterOptions{ - entity: "issue", - state: state, - assignee: assignee, - labels: labels, - author: author, - mention: mention, - milestone: milestone, + openURL, err := shared.ListURLWithQuery(issueListURL, shared.FilterOptions{ + Entity: "issue", + State: state, + Assignee: assignee, + Labels: labels, + Author: author, + Mention: mention, + Milestone: milestone, }) if err != nil { return err @@ -259,7 +208,7 @@ func issueList(cmd *cobra.Command, args []string) error { } }) - title := listHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, hasFilters) + title := shared.ListHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, hasFilters) if connectedToTerminal(cmd) { fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) } @@ -362,25 +311,6 @@ func issueStateTitleWithColor(state string) string { return colorFunc(strings.Title(strings.ToLower(state))) } -func listHeader(repoName string, itemName string, matchCount int, totalMatchCount int, hasFilters bool) string { - if totalMatchCount == 0 { - if hasFilters { - return fmt.Sprintf("No %ss match your search in %s", itemName, repoName) - } - return fmt.Sprintf("There are no open %ss in %s", itemName, repoName) - } - - if hasFilters { - matchVerb := "match" - if totalMatchCount == 1 { - matchVerb = "matches" - } - return fmt.Sprintf("Showing %d of %s in %s that %s your search", matchCount, utils.Pluralize(totalMatchCount, itemName), repoName, matchVerb) - } - - return fmt.Sprintf("Showing %d of %s in %s", matchCount, utils.Pluralize(totalMatchCount, fmt.Sprintf("open %s", itemName)), repoName) -} - func printRawIssuePreview(out io.Writer, issue *api.Issue) error { assignees := issueAssigneeList(*issue) labels := issueLabelList(*issue) @@ -508,11 +438,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { if isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") if title != "" || body != "" { - milestone := "" - if len(milestoneTitles) > 0 { - milestone = milestoneTitles[0] - } - openURL, err = withPrAndIssueQueryParams(openURL, title, body, assignees, labelNames, projectNames, milestone) + openURL, err = shared.WithPrAndIssueQueryParams(openURL, title, body, assignees, labelNames, projectNames, milestoneTitles) if err != nil { return err } @@ -535,9 +461,9 @@ func issueCreate(cmd *cobra.Command, args []string) error { return fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(baseRepo)) } - action := SubmitAction - tb := issueMetadataState{ - Type: issueMetadata, + action := shared.SubmitAction + tb := shared.IssueMetadataState{ + Type: shared.IssueMetadata, Assignees: assignees, Labels: labelNames, Projects: projectNames, @@ -558,14 +484,20 @@ func issueCreate(cmd *cobra.Command, args []string) error { legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "ISSUE_TEMPLATE") } } - err := titleBodySurvey(cmd, &tb, apiClient, baseRepo, title, body, defaults{}, nonLegacyTemplateFiles, legacyTemplateFile, false, repo.ViewerCanTriage()) + + editorCommand, err := cmdutil.DetermineEditor(ctx.Config) + if err != nil { + return err + } + + err = shared.TitleBodySurvey(defaultStreams, editorCommand, &tb, apiClient, baseRepo, title, body, shared.Defaults{}, nonLegacyTemplateFiles, legacyTemplateFile, false, repo.ViewerCanTriage()) if err != nil { return fmt.Errorf("could not collect title and/or body: %w", err) } action = tb.Action - if tb.Action == CancelAction { + if tb.Action == shared.CancelAction { fmt.Fprintln(cmd.ErrOrStderr(), "Discarding.") return nil @@ -583,26 +515,22 @@ func issueCreate(cmd *cobra.Command, args []string) error { } } - if action == PreviewAction { + if action == shared.PreviewAction { openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") - milestone := "" - if len(milestoneTitles) > 0 { - milestone = milestoneTitles[0] - } - openURL, err = withPrAndIssueQueryParams(openURL, title, body, assignees, labelNames, projectNames, milestone) + openURL, err = shared.WithPrAndIssueQueryParams(openURL, title, body, assignees, labelNames, projectNames, milestoneTitles) if err != nil { return err } // TODO could exceed max url length for explorer fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) return utils.OpenInBrowser(openURL) - } else if action == SubmitAction { + } else if action == shared.SubmitAction { params := map[string]interface{}{ "title": title, "body": body, } - err = addMetadataToIssueParams(apiClient, baseRepo, params, &tb) + err = shared.AddMetadataToIssueParams(apiClient, baseRepo, params, &tb) if err != nil { return err } @@ -620,84 +548,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *issueMetadataState) error { - if !tb.HasMetadata() { - return nil - } - - if tb.MetadataResult == nil { - resolveInput := api.RepoResolveInput{ - Reviewers: tb.Reviewers, - Assignees: tb.Assignees, - Labels: tb.Labels, - Projects: tb.Projects, - Milestones: tb.Milestones, - } - - var err error - tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) - if err != nil { - return err - } - } - - assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) - if err != nil { - return fmt.Errorf("could not assign user: %w", err) - } - params["assigneeIds"] = assigneeIDs - - labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) - if err != nil { - return fmt.Errorf("could not add label: %w", err) - } - params["labelIds"] = labelIDs - - projectIDs, err := tb.MetadataResult.ProjectsToIDs(tb.Projects) - if err != nil { - return fmt.Errorf("could not add to project: %w", err) - } - params["projectIds"] = projectIDs - - if len(tb.Milestones) > 0 { - milestoneID, err := tb.MetadataResult.MilestoneToID(tb.Milestones[0]) - if err != nil { - return fmt.Errorf("could not add to milestone '%s': %w", tb.Milestones[0], err) - } - params["milestoneId"] = milestoneID - } - - if len(tb.Reviewers) == 0 { - return nil - } - - var userReviewers []string - var teamReviewers []string - for _, r := range tb.Reviewers { - if strings.ContainsRune(r, '/') { - teamReviewers = append(teamReviewers, r) - } else { - userReviewers = append(userReviewers, r) - } - } - - userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["userReviewerIds"] = userReviewerIDs - - teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["teamReviewerIds"] = teamReviewerIDs - - return nil -} - func printIssues(w io.Writer, prefix string, totalCount int, issues []api.Issue) { - table := utils.NewTablePrinter(w) + io := &iostreams.IOStreams{Out: w} + io.SetStdoutTTY(utils.IsTerminal(w)) + table := utils.NewTablePrinter(io) for _, issue := range issues { issueNum := strconv.Itoa(issue.Number) if table.IsTTY() { diff --git a/command/issue_test.go b/command/issue_test.go index 8cde92d6e..16d613d72 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -803,117 +803,6 @@ func TestIssueCreate_webTitleBody(t *testing.T) { eq(t, output.String(), "Opening github.com/OWNER/REPO/issues/new in your browser.\n") } -func Test_listHeader(t *testing.T) { - type args struct { - repoName string - itemName string - matchCount int - totalMatchCount int - hasFilters bool - } - tests := []struct { - name string - args args - want string - }{ - { - name: "no results", - args: args{ - repoName: "REPO", - itemName: "table", - matchCount: 0, - totalMatchCount: 0, - hasFilters: false, - }, - want: "There are no open tables in REPO", - }, - { - name: "no matches after filters", - args: args{ - repoName: "REPO", - itemName: "Luftballon", - matchCount: 0, - totalMatchCount: 0, - hasFilters: true, - }, - want: "No Luftballons match your search in REPO", - }, - { - name: "one result", - args: args{ - repoName: "REPO", - itemName: "genie", - matchCount: 1, - totalMatchCount: 23, - hasFilters: false, - }, - want: "Showing 1 of 23 open genies in REPO", - }, - { - name: "one result after filters", - args: args{ - repoName: "REPO", - itemName: "tiny cup", - matchCount: 1, - totalMatchCount: 23, - hasFilters: true, - }, - want: "Showing 1 of 23 tiny cups in REPO that match your search", - }, - { - name: "one result in total", - args: args{ - repoName: "REPO", - itemName: "chip", - matchCount: 1, - totalMatchCount: 1, - hasFilters: false, - }, - want: "Showing 1 of 1 open chip in REPO", - }, - { - name: "one result in total after filters", - args: args{ - repoName: "REPO", - itemName: "spicy noodle", - matchCount: 1, - totalMatchCount: 1, - hasFilters: true, - }, - want: "Showing 1 of 1 spicy noodle in REPO that matches your search", - }, - { - name: "multiple results", - args: args{ - repoName: "REPO", - itemName: "plant", - matchCount: 4, - totalMatchCount: 23, - hasFilters: false, - }, - want: "Showing 4 of 23 open plants in REPO", - }, - { - name: "multiple results after filters", - args: args{ - repoName: "REPO", - itemName: "boomerang", - matchCount: 4, - totalMatchCount: 23, - hasFilters: true, - }, - want: "Showing 4 of 23 boomerangs in REPO that match your search", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := listHeader(tt.args.repoName, tt.args.itemName, tt.args.matchCount, tt.args.totalMatchCount, tt.args.hasFilters); got != tt.want { - t.Errorf("listHeader() = %v, want %v", got, tt.want) - } - }) - } -} - func TestIssueStateTitleWithColor(t *testing.T) { tests := map[string]struct { state string @@ -1071,71 +960,3 @@ func TestIssueReopen_issuesDisabled(t *testing.T) { t.Fatalf("got error: %v", err) } } - -func Test_listURLWithQuery(t *testing.T) { - type args struct { - listURL string - options filterOptions - } - tests := []struct { - name string - args args - want string - wantErr bool - }{ - { - name: "blank", - args: args{ - listURL: "https://example.com/path?a=b", - options: filterOptions{ - entity: "issue", - state: "open", - }, - }, - want: "https://example.com/path?a=b&q=is%3Aissue+is%3Aopen", - wantErr: false, - }, - { - name: "all", - args: args{ - listURL: "https://example.com/path", - options: filterOptions{ - entity: "issue", - state: "open", - assignee: "bo", - author: "ka", - baseBranch: "trunk", - mention: "nu", - }, - }, - want: "https://example.com/path?q=is%3Aissue+is%3Aopen+assignee%3Abo+author%3Aka+base%3Atrunk+mentions%3Anu", - wantErr: false, - }, - { - name: "spaces in values", - args: args{ - listURL: "https://example.com/path", - options: filterOptions{ - entity: "pr", - state: "open", - labels: []string{"docs", "help wanted"}, - milestone: `Codename "What Was Missing"`, - }, - }, - want: "https://example.com/path?q=is%3Apr+is%3Aopen+label%3Adocs+label%3A%22help+wanted%22+milestone%3A%22Codename+%5C%22What+Was+Missing%5C%22%22", - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := listURLWithQuery(tt.args.listURL, tt.args.options) - if (err != nil) != tt.wantErr { - t.Errorf("listURLWithQuery() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("listURLWithQuery() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/command/pr.go b/command/pr.go deleted file mode 100644 index 49604d3b4..000000000 --- a/command/pr.go +++ /dev/null @@ -1,306 +0,0 @@ -package command - -import ( - "fmt" - "strconv" - - "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/api" - "github.com/cli/cli/internal/ghrepo" - "github.com/cli/cli/pkg/cmd/pr/shared" - "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/pkg/text" - "github.com/cli/cli/utils" - "github.com/spf13/cobra" - "github.com/spf13/pflag" -) - -func init() { - prCmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") - - RootCmd.AddCommand(prCmd) - prCmd.AddCommand(prCreateCmd) - prCmd.AddCommand(prCloseCmd) - prCmd.AddCommand(prReopenCmd) - prCmd.AddCommand(prReadyCmd) - - prCmd.AddCommand(prListCmd) - prListCmd.Flags().BoolP("web", "w", false, "Open the browser to list the pull request(s)") - prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") - prListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|merged|all}") - prListCmd.Flags().StringP("base", "B", "", "Filter by base branch") - prListCmd.Flags().StringSliceP("label", "l", nil, "Filter by labels") - prListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") -} - -var prCmd = &cobra.Command{ - Use: "pr ", - Short: "Create, view, and checkout pull requests", - Long: `Work with GitHub pull requests`, - Example: heredoc.Doc(` - $ gh pr checkout 353 - $ gh pr create --fill - $ gh pr view --web - `), - Annotations: map[string]string{ - "IsCore": "true", - "help:arguments": `A pull request can be supplied as argument in any of the following formats: -- by number, e.g. "123"; -- by URL, e.g. "https://github.com/OWNER/REPO/pull/123"; or -- by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1".`}, -} -var prListCmd = &cobra.Command{ - Use: "list", - Short: "List and filter pull requests in this repository", - Args: cmdutil.NoArgsQuoteReminder, - Example: heredoc.Doc(` - $ gh pr list --limit 999 - $ gh pr list --state closed - $ gh pr list --label "priority 1" --label "bug" - $ gh pr list --web - `), - RunE: prList, -} -var prCloseCmd = &cobra.Command{ - Use: "close { | | }", - Short: "Close a pull request", - Args: cobra.ExactArgs(1), - RunE: prClose, -} -var prReopenCmd = &cobra.Command{ - Use: "reopen { | | }", - Short: "Reopen a pull request", - Args: cobra.ExactArgs(1), - RunE: prReopen, -} -var prReadyCmd = &cobra.Command{ - Use: "ready [ | | ]", - Short: "Mark a pull request as ready for review", - Args: cobra.MaximumNArgs(1), - RunE: prReady, -} - -func prList(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return err - } - - web, err := cmd.Flags().GetBool("web") - if err != nil { - return err - } - - limit, err := cmd.Flags().GetInt("limit") - if err != nil { - return err - } - if limit <= 0 { - return fmt.Errorf("invalid limit: %v", limit) - } - - state, err := cmd.Flags().GetString("state") - if err != nil { - return err - } - baseBranch, err := cmd.Flags().GetString("base") - if err != nil { - return err - } - labels, err := cmd.Flags().GetStringSlice("label") - if err != nil { - return err - } - assignee, err := cmd.Flags().GetString("assignee") - if err != nil { - return err - } - - if web { - prListURL := ghrepo.GenerateRepoURL(baseRepo, "pulls") - openURL, err := listURLWithQuery(prListURL, filterOptions{ - entity: "pr", - state: state, - assignee: assignee, - labels: labels, - baseBranch: baseBranch, - }) - if err != nil { - return err - } - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) - return utils.OpenInBrowser(openURL) - } - - var graphqlState []string - switch state { - case "open": - graphqlState = []string{"OPEN"} - case "closed": - graphqlState = []string{"CLOSED", "MERGED"} - case "merged": - graphqlState = []string{"MERGED"} - case "all": - graphqlState = []string{"OPEN", "CLOSED", "MERGED"} - default: - return fmt.Errorf("invalid state: %s", state) - } - - params := map[string]interface{}{ - "state": graphqlState, - } - if len(labels) > 0 { - params["labels"] = labels - } - if baseBranch != "" { - params["baseBranch"] = baseBranch - } - if assignee != "" { - params["assignee"] = assignee - } - - listResult, err := api.PullRequestList(apiClient, baseRepo, params, limit) - if err != nil { - return err - } - - hasFilters := false - cmd.Flags().Visit(func(f *pflag.Flag) { - switch f.Name { - case "state", "label", "base", "assignee": - hasFilters = true - } - }) - - title := listHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, hasFilters) - if connectedToTerminal(cmd) { - fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) - } - - table := utils.NewTablePrinter(cmd.OutOrStdout()) - for _, pr := range listResult.PullRequests { - prNum := strconv.Itoa(pr.Number) - if table.IsTTY() { - prNum = "#" + prNum - } - table.AddField(prNum, nil, shared.ColorFuncForPR(pr)) - table.AddField(text.ReplaceExcessiveWhitespace(pr.Title), nil, nil) - table.AddField(pr.HeadLabel(), nil, utils.Cyan) - if !table.IsTTY() { - table.AddField(prStateWithDraft(&pr), nil, nil) - } - table.EndRow() - } - err = table.Render() - if err != nil { - return err - } - - return nil -} - -func prClose(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return err - } - - if pr.State == "MERGED" { - err := fmt.Errorf("%s Pull request #%d (%s) can't be closed because it was already merged", utils.Red("!"), pr.Number, pr.Title) - return err - } else if pr.Closed { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d (%s) is already closed\n", utils.Yellow("!"), pr.Number, pr.Title) - return nil - } - - err = api.PullRequestClose(apiClient, baseRepo, pr) - if err != nil { - return fmt.Errorf("API call failed: %w", err) - } - - fmt.Fprintf(colorableErr(cmd), "%s Closed pull request #%d (%s)\n", utils.Red("✔"), pr.Number, pr.Title) - - return nil -} - -func prReopen(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return err - } - - if pr.State == "MERGED" { - err := fmt.Errorf("%s Pull request #%d (%s) can't be reopened because it was already merged", utils.Red("!"), pr.Number, pr.Title) - return err - } - - if !pr.Closed { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d (%s) is already open\n", utils.Yellow("!"), pr.Number, pr.Title) - return nil - } - - err = api.PullRequestReopen(apiClient, baseRepo, pr) - if err != nil { - return fmt.Errorf("API call failed: %w", err) - } - - fmt.Fprintf(colorableErr(cmd), "%s Reopened pull request #%d (%s)\n", utils.Green("✔"), pr.Number, pr.Title) - - return nil -} - -func prStateWithDraft(pr *api.PullRequest) string { - if pr.IsDraft && pr.State == "OPEN" { - return "DRAFT" - } - - return pr.State -} - -func prReady(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return err - } - - if pr.Closed { - err := fmt.Errorf("%s Pull request #%d is closed. Only draft pull requests can be marked as \"ready for review\"", utils.Red("!"), pr.Number) - return err - } else if !pr.IsDraft { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already \"ready for review\"\n", utils.Yellow("!"), pr.Number) - return nil - } - - err = api.PullRequestReady(apiClient, baseRepo, pr) - if err != nil { - return fmt.Errorf("API call failed: %w", err) - } - - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is marked as \"ready for review\"\n", utils.Green("✔"), pr.Number) - - return nil -} diff --git a/command/pr_lookup.go b/command/pr_lookup.go deleted file mode 100644 index 727259f7b..000000000 --- a/command/pr_lookup.go +++ /dev/null @@ -1,34 +0,0 @@ -package command - -import ( - "fmt" - - "github.com/cli/cli/api" - "github.com/cli/cli/context" - "github.com/cli/cli/internal/ghrepo" - "github.com/cli/cli/pkg/cmd/pr/shared" - "github.com/spf13/cobra" -) - -func prFromArgs(ctx context.Context, apiClient *api.Client, cmd *cobra.Command, args []string) (*api.PullRequest, ghrepo.Interface, error) { - var arg string - if len(args) > 0 { - arg = args[0] - } - - return shared.PRFromArgs( - apiClient, - func() (ghrepo.Interface, error) { - repo, err := determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return nil, fmt.Errorf("could not determine base repo: %w", err) - } - return repo, nil - }, - func() (string, error) { - return ctx.Branch() - }, - func() (context.Remotes, error) { - return ctx.Remotes() - }, arg) -} diff --git a/command/pr_test.go b/command/pr_test.go deleted file mode 100644 index f02a2e9f2..000000000 --- a/command/pr_test.go +++ /dev/null @@ -1,406 +0,0 @@ -package command - -import ( - "bytes" - "os/exec" - "reflect" - "regexp" - "strings" - "testing" - - "github.com/cli/cli/internal/run" - "github.com/cli/cli/pkg/httpmock" - "github.com/cli/cli/test" - "github.com/stretchr/testify/assert" -) - -func eq(t *testing.T, got interface{}, expected interface{}) { - t.Helper() - if !reflect.DeepEqual(got, expected) { - t.Errorf("expected: %v, got: %v", expected, got) - } -} - -func TestPRList(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("../test/fixtures/prList.json")) - - output, err := RunCommand("pr list") - if err != nil { - t.Fatal(err) - } - - assert.Equal(t, ` -Showing 3 of 3 open pull requests in OWNER/REPO - -`, output.Stderr()) - - lines := strings.Split(output.String(), "\n") - res := []*regexp.Regexp{ - regexp.MustCompile(`#32.*New feature.*feature`), - regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), - regexp.MustCompile(`#28.*Improve documentation.*docs`), - } - - for i, r := range res { - if !r.MatchString(lines[i]) { - t.Errorf("%s did not match %s", lines[i], r) - } - } -} - -func TestPRList_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(false)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("../test/fixtures/prList.json")) - - output, err := RunCommand("pr list") - if err != nil { - t.Fatal(err) - } - - assert.Equal(t, "", output.Stderr()) - - assert.Equal(t, `32 New feature feature DRAFT -29 Fixed bad bug hubot:bug-fix OPEN -28 Improve documentation docs MERGED -`, output.String()) -} - -func TestPRList_filtering(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query PullRequestList\b`), - httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { - assert.Equal(t, []interface{}{"OPEN", "CLOSED", "MERGED"}, params["state"].([]interface{})) - assert.Equal(t, []interface{}{"one", "two", "three"}, params["labels"].([]interface{})) - })) - - output, err := RunCommand(`pr list -s all -l one,two -l three`) - if err != nil { - t.Fatal(err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), ` -No pull requests match your search in OWNER/REPO - -`) -} - -func TestPRList_filteringRemoveDuplicate(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query PullRequestList\b`), - httpmock.FileResponse("../test/fixtures/prListWithDuplicates.json")) - - output, err := RunCommand("pr list -l one,two") - if err != nil { - t.Fatal(err) - } - - lines := strings.Split(output.String(), "\n") - - res := []*regexp.Regexp{ - regexp.MustCompile(`#32.*New feature.*feature`), - regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), - regexp.MustCompile(`#28.*Improve documentation.*docs`), - } - - for i, r := range res { - if !r.MatchString(lines[i]) { - t.Errorf("%s did not match %s", lines[i], r) - } - } -} - -func TestPRList_filteringClosed(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query PullRequestList\b`), - httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { - assert.Equal(t, []interface{}{"CLOSED", "MERGED"}, params["state"].([]interface{})) - })) - - _, err := RunCommand(`pr list -s closed`) - if err != nil { - t.Fatal(err) - } -} - -func TestPRList_filteringAssignee(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query PullRequestList\b`), - httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { - assert.Equal(t, `repo:OWNER/REPO assignee:hubot is:pr sort:created-desc is:merged label:"needs tests" base:"develop"`, params["q"].(string)) - })) - - _, err := RunCommand(`pr list -s merged -l "needs tests" -a hubot -B develop`) - if err != nil { - t.Fatal(err) - } -} - -func TestPRList_filteringAssigneeLabels(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - initFakeHTTP() - - _, err := RunCommand(`pr list -l one,two -a hubot`) - if err == nil && err.Error() != "multiple labels with --assignee are not supported" { - t.Fatal(err) - } -} - -func TestPRList_withInvalidLimitFlag(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - initFakeHTTP() - - _, err := RunCommand(`pr list --limit=0`) - if err == nil && err.Error() != "invalid limit: 0" { - t.Errorf("error running command `issue list`: %v", err) - } -} - -func TestPRList_web(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - var seenCmd *exec.Cmd - restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { - seenCmd = cmd - return &test.OutputStub{} - }) - defer restoreCmd() - - output, err := RunCommand("pr list --web -a peter -l bug -l docs -L 10 -s merged -B trunk") - if err != nil { - t.Errorf("error running command `pr list` with `--web` flag: %v", err) - } - - expectedURL := "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk" - - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/pulls in your browser.\n") - - if seenCmd == nil { - t.Fatal("expected a command to run") - } - url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, expectedURL) -} - -func TestPrClose(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 96, "title": "The title of the PR" } - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr close 96") - if err != nil { - t.Fatalf("error running command `pr close`: %v", err) - } - - r := regexp.MustCompile(`Closed pull request #96 \(The title of the PR\)`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPrClose_alreadyClosed(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 101, "title": "The title of the PR", "closed": true } - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr close 101") - if err != nil { - t.Fatalf("error running command `pr close`: %v", err) - } - - r := regexp.MustCompile(`Pull request #101 \(The title of the PR\) is already closed`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReopen(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "closed": true} - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr reopen 666") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Reopened pull request #666 \(The title of the PR\)`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReopen_alreadyOpen(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "closed": false} - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr reopen 666") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) is already open`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReopen_alreadyMerged(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "closed": true, "state": "MERGED"} - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr reopen 666") - if err == nil { - t.Fatalf("expected an error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) can't be reopened because it was already merged`) - - if !r.MatchString(err.Error()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReady(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 444, "closed": false, "isDraft": true} - } } } - `)) - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr ready 444") - if err != nil { - t.Fatalf("error running command `pr ready`: %v", err) - } - - r := regexp.MustCompile(`Pull request #444 is marked as "ready for review"`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReady_alreadyReady(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 445, "closed": false, "isDraft": false} - } } } - `)) - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - output, err := RunCommand("pr ready 445") - if err != nil { - t.Fatalf("error running command `pr ready`: %v", err) - } - - r := regexp.MustCompile(`Pull request #445 is already "ready for review"`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReady_closed(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 446, "closed": true, "isDraft": true} - } } } - `)) - http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - - _, err := RunCommand("pr ready 446") - if err == nil { - t.Fatalf("expected an error running command `pr ready` on a review that is closed!: %v", err) - } - - r := regexp.MustCompile(`Pull request #446 is closed. Only draft pull requests can be marked as "ready for review"`) - - if !r.MatchString(err.Error()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, err.Error()) - } -} diff --git a/command/root.go b/command/root.go index 45de50f8e..9eb1c9287 100644 --- a/command/root.go +++ b/command/root.go @@ -26,12 +26,7 @@ import ( authLoginCmd "github.com/cli/cli/pkg/cmd/auth/login" authLogoutCmd "github.com/cli/cli/pkg/cmd/auth/logout" gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" - prCheckoutCmd "github.com/cli/cli/pkg/cmd/pr/checkout" - prDiffCmd "github.com/cli/cli/pkg/cmd/pr/diff" - prMergeCmd "github.com/cli/cli/pkg/cmd/pr/merge" - prReviewCmd "github.com/cli/cli/pkg/cmd/pr/review" - prStatusCmd "github.com/cli/cli/pkg/cmd/pr/status" - prViewCmd "github.com/cli/cli/pkg/cmd/pr/view" + prCmd "github.com/cli/cli/pkg/cmd/pr" repoCmd "github.com/cli/cli/pkg/cmd/repo" repoCloneCmd "github.com/cli/cli/pkg/cmd/repo/clone" repoCreateCmd "github.com/cli/cli/pkg/cmd/repo/create" @@ -180,13 +175,7 @@ func init() { repoCmd.Cmd.AddCommand(repoCreateCmd.NewCmdCreate(cmdFactory, nil)) repoCmd.Cmd.AddCommand(creditsCmd.NewCmdRepoCredits(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prReviewCmd.NewCmdReview(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prDiffCmd.NewCmdDiff(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prCheckoutCmd.NewCmdCheckout(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prViewCmd.NewCmdView(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prMergeCmd.NewCmdMerge(&repoResolvingCmdFactory, nil)) - prCmd.AddCommand(prStatusCmd.NewCmdStatus(&repoResolvingCmdFactory, nil)) - + RootCmd.AddCommand(prCmd.NewCmdPR(&repoResolvingCmdFactory)) RootCmd.AddCommand(creditsCmd.NewCmdCredits(cmdFactory, nil)) } @@ -405,41 +394,6 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } -// TODO there is a parallel implementation for isolated commands -func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { - ctx := contextForCommand(cmd) - - var protocol string - cfg, err := ctx.Config() - if err != nil { - fmt.Fprintf(colorableErr(cmd), "%s failed to load config: %s. using defaults\n", utils.Yellow("!"), err) - } else { - protocol, _ = cfg.Get(repo.RepoHost(), "git_protocol") - } - - if protocol == "ssh" { - return fmt.Sprintf("git@%s:%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) - } - - return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) -} - -// TODO there is a parallel implementation for isolated commands -func determineEditor(cmd *cobra.Command) (string, error) { - editorCommand := os.Getenv("GH_EDITOR") - if editorCommand == "" { - ctx := contextForCommand(cmd) - cfg, err := ctx.Config() - if err != nil { - return "", fmt.Errorf("could not read config: %w", err) - } - // TODO: consider supporting setting an editor per GHE host - editorCommand, _ = cfg.Get(ghinstance.Default(), "editor") - } - - return editorCommand, nil -} - func ExecuteShellAlias(args []string) error { externalCmd := exec.Command(args[0], args[1:]...) externalCmd.Stderr = os.Stderr diff --git a/command/testing.go b/command/testing.go index 367d510bb..fe9b76ee7 100644 --- a/command/testing.go +++ b/command/testing.go @@ -3,6 +3,8 @@ package command import ( "bytes" "fmt" + "reflect" + "testing" "github.com/cli/cli/api" "github.com/cli/cli/context" @@ -13,6 +15,13 @@ import ( "github.com/spf13/pflag" ) +func eq(t *testing.T, got interface{}, expected interface{}) { + t.Helper() + if !reflect.DeepEqual(got, expected) { + t.Errorf("expected: %v, got: %v", expected, got) + } +} + const defaultTestConfig = `hosts: github.com: user: OWNER diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 20eef580c..23c4bb70b 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -36,7 +36,6 @@ func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobr IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -51,6 +50,9 @@ func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobr return nil }, RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + if len(args) > 0 { opts.SelectorArg = args[0] } diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go new file mode 100644 index 000000000..2fba67e06 --- /dev/null +++ b/pkg/cmd/pr/close/close.go @@ -0,0 +1,83 @@ +package close + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type CloseOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + + SelectorArg string +} + +func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Command { + opts := &CloseOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + } + + cmd := &cobra.Command{ + Use: "close { | | }", + Short: "Close a pull request", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return closeRun(opts) + }, + } + + return cmd +} + +func closeRun(opts *CloseOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, nil, nil, opts.SelectorArg) + if err != nil { + return err + } + + if pr.State == "MERGED" { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be closed because it was already merged", utils.Red("!"), pr.Number, pr.Title) + return cmdutil.SilentError + } else if pr.Closed { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already closed\n", utils.Yellow("!"), pr.Number, pr.Title) + return nil + } + + err = api.PullRequestClose(apiClient, baseRepo, pr) + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Closed pull request #%d (%s)\n", utils.Green("✔"), pr.Number, pr.Title) + + return nil +} diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go new file mode 100644 index 000000000..1bb530637 --- /dev/null +++ b/pkg/cmd/pr/close/close_test.go @@ -0,0 +1,101 @@ +package close + +import ( + "bytes" + "io/ioutil" + "net/http" + "regexp" + "testing" + + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/test" + "github.com/google/shlex" +) + +func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + cmd := NewCmdClose(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPrClose(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 96, "title": "The title of the PR" } + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := runCommand(http, true, "96") + if err != nil { + t.Fatalf("error running command `pr close`: %v", err) + } + + r := regexp.MustCompile(`Closed pull request #96 \(The title of the PR\)`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPrClose_alreadyClosed(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 101, "title": "The title of the PR", "closed": true } + } } } + `)) + + output, err := runCommand(http, true, "101") + if err != nil { + t.Fatalf("error running command `pr close`: %v", err) + } + + r := regexp.MustCompile(`Pull request #101 \(The title of the PR\) is already closed`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} diff --git a/command/pr_create.go b/pkg/cmd/pr/create/create.go similarity index 54% rename from command/pr_create.go rename to pkg/cmd/pr/create/create.go index b9efca0fa..c9e1fb6ab 100644 --- a/command/pr_create.go +++ b/pkg/cmd/pr/create/create.go @@ -1,9 +1,9 @@ -package command +package create import ( "errors" "fmt" - "net/url" + "net/http" "strings" "time" @@ -11,60 +11,116 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/githubtemplate" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) -type defaults struct { - Title string - Body string +type CreateOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + Remotes func() (context.Remotes, error) + Branch func() (string, error) + + RepoOverride string + + Autofill bool + WebMode bool + + IsDraft bool + Title string + TitleProvided bool + Body string + BodyProvided bool + BaseBranch string + + Reviewers []string + Assignees []string + Labels []string + Projects []string + Milestone string } -func computeDefaults(baseRef, headRef string) (defaults, error) { - commits, err := git.Commits(baseRef, headRef) +func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command { + opts := &CreateOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + Remotes: f.Remotes, + Branch: f.Branch, + } + + cmd := &cobra.Command{ + Use: "create", + Short: "Create a pull request", + Example: heredoc.Doc(` + $ gh pr create --title "The bug is fixed" --body "Everything works again" + $ gh issue create --label "bug,help wanted" + $ gh issue create --label bug --label "help wanted" + $ gh pr create --reviewer monalisa,hubot + $ gh pr create --project "Roadmap" + $ gh pr create --base develop + `), + Args: cmdutil.NoArgsQuoteReminder, + RunE: func(cmd *cobra.Command, args []string) error { + opts.TitleProvided = cmd.Flags().Changed("title") + opts.BodyProvided = cmd.Flags().Changed("body") + opts.RepoOverride, _ = cmd.Flags().GetString("repo") + + isTerminal := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() + if !isTerminal && !opts.WebMode && !opts.TitleProvided && !opts.Autofill { + return errors.New("--title or --fill required when not attached to a terminal") + } + + if opts.IsDraft && opts.WebMode { + return errors.New("the --draft flag is not supported with --web") + } + if len(opts.Reviewers) > 0 && opts.WebMode { + return errors.New("the --reviewer flag is not supported with --web") + } + + if runF != nil { + return runF(opts) + } + return createRun(opts) + }, + } + + fl := cmd.Flags() + fl.BoolVarP(&opts.IsDraft, "draft", "d", false, "Mark pull request as a draft") + fl.StringVarP(&opts.Title, "title", "t", "", "Supply a title. Will prompt for one otherwise.") + fl.StringVarP(&opts.Body, "body", "b", "", "Supply a body. Will prompt for one otherwise.") + fl.StringVarP(&opts.BaseBranch, "base", "B", "", "The branch into which you want your code merged") + fl.BoolVarP(&opts.WebMode, "web", "w", false, "Open the web browser to create a pull request") + fl.BoolVarP(&opts.Autofill, "fill", "f", false, "Do not prompt for title/body and just use commit info") + fl.StringSliceVarP(&opts.Reviewers, "reviewer", "r", nil, "Request reviews from people by their `login`") + fl.StringSliceVarP(&opts.Assignees, "assignee", "a", nil, "Assign people by their `login`") + fl.StringSliceVarP(&opts.Labels, "label", "l", nil, "Add labels by `name`") + fl.StringSliceVarP(&opts.Projects, "project", "p", nil, "Add the pull request to projects by `name`") + fl.StringVarP(&opts.Milestone, "milestone", "m", "", "Add the pull request to a milestone by `name`") + + return cmd +} + +func createRun(opts *CreateOptions) error { + httpClient, err := opts.HttpClient() if err != nil { - return defaults{}, err + return err } + client := api.NewClientFromHTTP(httpClient) - out := defaults{} - - if len(commits) == 1 { - out.Title = commits[0].Title - body, err := git.CommitBody(commits[0].Sha) - if err != nil { - return defaults{}, err - } - out.Body = body - } else { - out.Title = utils.Humanize(headRef) - - body := "" - for i := len(commits) - 1; i >= 0; i-- { - body += fmt.Sprintf("- %s\n", commits[i].Title) - } - out.Body = body - } - - return out, nil -} - -func prCreate(cmd *cobra.Command, _ []string) error { - ctx := contextForCommand(cmd) - remotes, err := ctx.Remotes() + remotes, err := opts.Remotes() if err != nil { return err } - client, err := apiClientForContext(ctx) - if err != nil { - return fmt.Errorf("could not initialize API client: %w", err) - } - - baseRepoOverride, _ := cmd.Flags().GetString("repo") - repoContext, err := context.ResolveRemotesToRepos(remotes, client, baseRepoOverride) + repoContext, err := context.ResolveRemotesToRepos(remotes, client, opts.RepoOverride) if err != nil { return err } @@ -74,7 +130,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("could not determine base repository: %w", err) } - headBranch, err := ctx.Branch() + headBranch, err := opts.Branch() if err != nil { return fmt.Errorf("could not determine the current branch: %w", err) } @@ -102,10 +158,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - baseBranch, err := cmd.Flags().GetString("base") - if err != nil { - return err - } + baseBranch := opts.BaseBranch if baseBranch == "" { baseBranch = baseRepo.DefaultBranchRef.Name } @@ -114,39 +167,12 @@ func prCreate(cmd *cobra.Command, _ []string) error { } if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { - fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) + fmt.Fprintf(opts.IO.ErrOut, "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) } - title, err := cmd.Flags().GetString("title") - if err != nil { - return fmt.Errorf("could not parse title: %w", err) - } - body, err := cmd.Flags().GetString("body") - if err != nil { - return fmt.Errorf("could not parse body: %w", err) - } - - reviewers, err := cmd.Flags().GetStringSlice("reviewer") - if err != nil { - return fmt.Errorf("could not parse reviewers: %w", err) - } - assignees, err := cmd.Flags().GetStringSlice("assignee") - if err != nil { - return fmt.Errorf("could not parse assignees: %w", err) - } - labelNames, err := cmd.Flags().GetStringSlice("label") - if err != nil { - return fmt.Errorf("could not parse labels: %w", err) - } - projectNames, err := cmd.Flags().GetStringSlice("project") - if err != nil { - return fmt.Errorf("could not parse projects: %w", err) - } var milestoneTitles []string - if milestoneTitle, err := cmd.Flags().GetString("milestone"); err != nil { - return fmt.Errorf("could not parse milestone: %w", err) - } else if milestoneTitle != "" { - milestoneTitles = append(milestoneTitles, milestoneTitle) + if opts.Milestone != "" { + milestoneTitles = []string{opts.Milestone} } baseTrackingBranch := baseBranch @@ -155,23 +181,16 @@ func prCreate(cmd *cobra.Command, _ []string) error { } defs, defaultsErr := computeDefaults(baseTrackingBranch, headBranch) - isWeb, err := cmd.Flags().GetBool("web") - if err != nil { - return fmt.Errorf("could not parse web: %q", err) - } + title := opts.Title + body := opts.Body - autofill, err := cmd.Flags().GetBool("fill") - if err != nil { - return fmt.Errorf("could not parse fill: %q", err) - } - - action := SubmitAction - if isWeb { - action = PreviewAction + action := shared.SubmitAction + if opts.WebMode { + action = shared.PreviewAction if (title == "" || body == "") && defaultsErr != nil { return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) } - } else if autofill { + } else if opts.Autofill { if defaultsErr != nil { return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) } @@ -179,7 +198,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { body = defs.Body } - if !isWeb { + if !opts.WebMode { headBranchLabel := headBranch if headRepo != nil && !ghrepo.IsSame(baseRepo, headRepo) { headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) @@ -194,46 +213,37 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - isDraft, err := cmd.Flags().GetBool("draft") - if err != nil { - return fmt.Errorf("could not parse draft: %w", err) - } + isTerminal := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - if !isWeb && !autofill { + if !opts.WebMode && !opts.Autofill { message := "\nCreating pull request for %s into %s in %s\n\n" - if isDraft { + if opts.IsDraft { message = "\nCreating draft pull request for %s into %s in %s\n\n" } - if connectedToTerminal(cmd) { - fmt.Fprintf(colorableErr(cmd), message, + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, message, utils.Cyan(headBranch), utils.Cyan(baseBranch), ghrepo.FullName(baseRepo)) if (title == "" || body == "") && defaultsErr != nil { - fmt.Fprintf(colorableErr(cmd), "%s warning: could not compute title or body defaults: %s\n", utils.Yellow("!"), defaultsErr) + fmt.Fprintf(opts.IO.ErrOut, "%s warning: could not compute title or body defaults: %s\n", utils.Yellow("!"), defaultsErr) } } } - tb := issueMetadataState{ - Type: prMetadata, - Reviewers: reviewers, - Assignees: assignees, - Labels: labelNames, - Projects: projectNames, + tb := shared.IssueMetadataState{ + Type: shared.PRMetadata, + Reviewers: opts.Reviewers, + Assignees: opts.Assignees, + Labels: opts.Labels, + Projects: opts.Projects, Milestones: milestoneTitles, } - if !connectedToTerminal(cmd) { - if !isWeb && (!cmd.Flags().Changed("title") && !autofill) { - return errors.New("--title or --fill required when not attached to a tty") - } - } + interactive := isTerminal && !(opts.TitleProvided && opts.BodyProvided) - interactive := connectedToTerminal(cmd) && !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) - - if !isWeb && !autofill && interactive { + if !opts.WebMode && !opts.Autofill && interactive { var nonLegacyTemplateFiles []string var legacyTemplateFile *string if rootDir, err := git.ToplevelDir(); err == nil { @@ -241,15 +251,21 @@ func prCreate(cmd *cobra.Command, _ []string) error { nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(rootDir, "PULL_REQUEST_TEMPLATE") legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "PULL_REQUEST_TEMPLATE") } - err := titleBodySurvey(cmd, &tb, client, baseRepo, title, body, defs, nonLegacyTemplateFiles, legacyTemplateFile, true, baseRepo.ViewerCanTriage()) + + editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + + err = shared.TitleBodySurvey(opts.IO, editorCommand, &tb, client, baseRepo, title, body, defs, nonLegacyTemplateFiles, legacyTemplateFile, true, baseRepo.ViewerCanTriage()) if err != nil { return fmt.Errorf("could not collect title and/or body: %w", err) } action = tb.Action - if action == CancelAction { - fmt.Fprintln(cmd.ErrOrStderr(), "Discarding.") + if action == shared.CancelAction { + fmt.Fprintln(opts.IO.ErrOut, "Discarding.") return nil } @@ -261,17 +277,10 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - if action == SubmitAction && title == "" { + if action == shared.SubmitAction && title == "" { return errors.New("pull request title must not be blank") } - if isDraft && isWeb { - return errors.New("the --draft flag is not supported with --web") - } - if len(reviewers) > 0 && isWeb { - return errors.New("the --reviewer flag is not supported with --web") - } - didForkRepo := false // if a head repository could not be determined so far, automatically create // one by forking the base repository @@ -303,7 +312,13 @@ func prCreate(cmd *cobra.Command, _ []string) error { // In either case, we want to add the head repo as a new git remote so we // can push to it. if headRemote == nil { - headRepoURL := formatRemoteURL(cmd, headRepo) + cfg, err := opts.Config() + if err != nil { + return err + } + cloneProtocol, _ := cfg.Get(headRepo.RepoHost(), "git_protocol") + + headRepoURL := ghrepo.FormatRemoteURL(headRepo, cloneProtocol) // TODO: prevent clashes with another remote of a same name gitRemote, err := git.AddRemote("fork", headRepoURL) @@ -326,7 +341,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { pushTries++ // first wait 2 seconds after forking, then 4s, then 6s waitSeconds := 2 * pushTries - fmt.Fprintf(cmd.ErrOrStderr(), "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) + fmt.Fprintf(opts.IO.ErrOut, "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) time.Sleep(time.Duration(waitSeconds) * time.Second) continue } @@ -336,16 +351,16 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - if action == SubmitAction { + if action == shared.SubmitAction { params := map[string]interface{}{ "title": title, "body": body, - "draft": isDraft, + "draft": opts.IsDraft, "baseRefName": baseBranch, "headRefName": headBranchLabel, } - err = addMetadataToIssueParams(client, baseRepo, params, &tb) + err = shared.AddMetadataToIssueParams(client, baseRepo, params, &tb) if err != nil { return err } @@ -355,19 +370,14 @@ func prCreate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("failed to create pull request: %w", err) } - fmt.Fprintln(cmd.OutOrStdout(), pr.URL) - } else if action == PreviewAction { - milestone := "" - if len(milestoneTitles) > 0 { - milestone = milestoneTitles[0] - } - openURL, err := generateCompareURL(baseRepo, baseBranch, headBranchLabel, title, body, assignees, labelNames, projectNames, milestone) + fmt.Fprintln(opts.IO.Out, pr.URL) + } else if action == shared.PreviewAction { + openURL, err := generateCompareURL(baseRepo, baseBranch, headBranchLabel, title, body, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) if err != nil { return err } - if connectedToTerminal(cmd) { - // TODO could exceed max url length for explorer - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } return utils.OpenInBrowser(openURL) } else { @@ -377,6 +387,34 @@ func prCreate(cmd *cobra.Command, _ []string) error { return nil } +func computeDefaults(baseRef, headRef string) (shared.Defaults, error) { + out := shared.Defaults{} + + commits, err := git.Commits(baseRef, headRef) + if err != nil { + return out, err + } + + if len(commits) == 1 { + out.Title = commits[0].Title + body, err := git.CommitBody(commits[0].Sha) + if err != nil { + return out, err + } + out.Body = body + } else { + out.Title = utils.Humanize(headRef) + + body := "" + for i := len(commits) - 1; i >= 0; i-- { + body += fmt.Sprintf("- %s\n", commits[i].Title) + } + out.Body = body + } + + return out, nil +} + func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.TrackingRef { refsForLookup := []string{"HEAD"} var trackingRefs []git.TrackingRef @@ -418,73 +456,11 @@ func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.Tr return nil } -func withPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, projects []string, milestone string) (string, error) { - u, err := url.Parse(baseURL) - if err != nil { - return "", err - } - q := u.Query() - if title != "" { - q.Set("title", title) - } - if body != "" { - q.Set("body", body) - } - if len(assignees) > 0 { - q.Set("assignees", strings.Join(assignees, ",")) - } - if len(labels) > 0 { - q.Set("labels", strings.Join(labels, ",")) - } - if len(projects) > 0 { - q.Set("projects", strings.Join(projects, ",")) - } - if milestone != "" { - q.Set("milestone", milestone) - } - u.RawQuery = q.Encode() - return u.String(), nil -} - -func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestone string) (string, error) { +func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestones []string) (string, error) { u := ghrepo.GenerateRepoURL(r, "compare/%s...%s?expand=1", base, head) - url, err := withPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestone) + url, err := shared.WithPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestones) if err != nil { return "", err } return url, nil } - -var prCreateCmd = &cobra.Command{ - Use: "create", - Short: "Create a pull request", - Args: cmdutil.NoArgsQuoteReminder, - RunE: prCreate, - Example: heredoc.Doc(` - $ gh pr create --title "The bug is fixed" --body "Everything works again" - $ gh issue create --label "bug,help wanted" - $ gh issue create --label bug --label "help wanted" - $ gh pr create --reviewer monalisa,hubot - $ gh pr create --project "Roadmap" - $ gh pr create --base develop - `), -} - -func init() { - prCreateCmd.Flags().BoolP("draft", "d", false, - "Mark pull request as a draft") - prCreateCmd.Flags().StringP("title", "t", "", - "Supply a title. Will prompt for one otherwise.") - prCreateCmd.Flags().StringP("body", "b", "", - "Supply a body. Will prompt for one otherwise.") - prCreateCmd.Flags().StringP("base", "B", "", - "The branch into which you want your code merged") - prCreateCmd.Flags().BoolP("web", "w", false, "Open the web browser to create a pull request") - prCreateCmd.Flags().BoolP("fill", "f", false, "Do not prompt for title/body and just use commit info") - - prCreateCmd.Flags().StringSliceP("reviewer", "r", nil, "Request reviews from people by their `login`") - prCreateCmd.Flags().StringSliceP("assignee", "a", nil, "Assign people by their `login`") - prCreateCmd.Flags().StringSliceP("label", "l", nil, "Add labels by `name`") - prCreateCmd.Flags().StringSliceP("project", "p", nil, "Add the pull request to projects by `name`") - prCreateCmd.Flags().StringP("milestone", "m", "", "Add the pull request to a milestone by `name`") -} diff --git a/command/pr_create_test.go b/pkg/cmd/pr/create/create_test.go similarity index 88% rename from command/pr_create_test.go rename to pkg/cmd/pr/create/create_test.go index c6daf8bef..e50a72c63 100644 --- a/command/pr_create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1,25 +1,93 @@ -package command +package create import ( "bytes" "encoding/json" "io/ioutil" + "net/http" + "reflect" "strings" "testing" "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/test" + "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func eq(t *testing.T, got interface{}, expected interface{}) { + t.Helper() + if !reflect.DeepEqual(got, expected) { + t.Errorf("expected: %v, got: %v", expected, got) + } +} + +func runCommand(rt http.RoundTripper, remotes context.Remotes, branch string, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + Remotes: func() (context.Remotes, error) { + if remotes != nil { + return remotes, nil + } + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Branch: func() (string, error) { + return branch, nil + }, + } + + cmd := NewCmdCreate(factory, nil) + cmd.PersistentFlags().StringP("repo", "R", "", "") + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func initFakeHTTP() *httpmock.Registry { + return &httpmock.Registry{} +} + func TestPRCreate_nontty_web(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -36,8 +104,8 @@ func TestPRCreate_nontty_web(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser - output, err := RunCommand(`pr create --web`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", false, `--web`) + require.NoError(t, err) eq(t, output.String(), "") eq(t, output.Stderr(), "") @@ -50,33 +118,23 @@ func TestPRCreate_nontty_web(t *testing.T) { } func TestPRCreate_nontty_insufficient_flags(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "forks": { "nodes": [ - ] } } } } - `)) - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + defer http.Verify(t) - output, err := RunCommand("pr create") + output, err := runCommand(http, nil, "feature", false, "") if err == nil { t.Fatal("expected error") } - assert.Equal(t, "--title or --fill required when not attached to a tty", err.Error()) + assert.Equal(t, "--title or --fill required when not attached to a terminal", err.Error()) assert.Equal(t, "", output.String()) } func TestPRCreate_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -101,8 +159,8 @@ func TestPRCreate_nontty(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t "my title" -b "my body"`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", false, `-t "my title" -b "my body"`) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { @@ -129,9 +187,9 @@ func TestPRCreate_nontty(t *testing.T) { } func TestPRCreate(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -156,8 +214,8 @@ func TestPRCreate(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t "my title" -b "my body"`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", true, `-t "my title" -b "my body"`) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { @@ -183,8 +241,6 @@ func TestPRCreate(t *testing.T) { } func TestPRCreate_metadata(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) @@ -301,16 +357,16 @@ func TestPRCreate_metadata(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh' -r hubot -r monalisa -r /core -r /robots`) + output, err := runCommand(http, nil, "feature", true, `-t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh' -r hubot -r monalisa -r /core -r /robots`) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } func TestPRCreate_withForking(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponseWithPermission("OWNER", "REPO", "READ") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -344,17 +400,17 @@ func TestPRCreate_withForking(t *testing.T) { cs.Stub("") // git remote add cs.Stub("") // git push - output, err := RunCommand(`pr create -t title -b body`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", true, `-t title -b body`) + require.NoError(t, err) eq(t, http.Requests[3].URL.Path, "/repos/OWNER/REPO/forks") eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } func TestPRCreate_alreadyExists(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -376,7 +432,7 @@ func TestPRCreate_alreadyExists(t *testing.T) { cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log - _, err := RunCommand(`pr create`) + _, err := runCommand(http, nil, "feature", true, ``) if err == nil { t.Fatal("error expected, got nil") } @@ -386,9 +442,9 @@ func TestPRCreate_alreadyExists(t *testing.T) { } func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -412,16 +468,16 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git rev-parse - _, err := RunCommand(`pr create -BanotherBase -t"cool" -b"nah"`) + _, err := runCommand(http, nil, "feature", true, `-BanotherBase -t"cool" -b"nah"`) if err != nil { t.Errorf("got unexpected error %q", err) } } func TestPRCreate_web(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -438,8 +494,8 @@ func TestPRCreate_web(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser - output, err := RunCommand(`pr create --web`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", true, `--web`) + require.NoError(t, err) eq(t, output.String(), "") eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/compare/master...feature in your browser.\n") @@ -451,9 +507,8 @@ func TestPRCreate_web(t *testing.T) { } func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -479,7 +534,7 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t "my title" -b "my body"`) + output, err := runCommand(http, nil, "feature", true, `-t "my title" -b "my body"`) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") @@ -487,17 +542,20 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { } func TestPRCreate_cross_repo_same_branch(t *testing.T) { - defer stubTerminal(true)() - ctx := context.NewBlank() - ctx.SetBranch("default") - ctx.SetRemotes(map[string]string{ - "origin": "OWNER/REPO", - "fork": "MYSELF/REPO", - }) - initContext = func() context.Context { - return ctx + remotes := context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + { + Remote: &git.Remote{Name: "fork"}, + Repo: ghrepo.New("MYSELF", "REPO"), + }, } + http := initFakeHTTP() + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` { "data": { "repo_000": { "id": "REPOID0", @@ -546,8 +604,8 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(`pr create -t "cross repo" -b "same branch"`) - eq(t, err, nil) + output, err := runCommand(http, remotes, "default", true, `-t "cross repo" -b "same branch"`) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { @@ -575,9 +633,9 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { } func TestPRCreate_survey_defaults_multicommit(t *testing.T) { - initBlankContext("", "OWNER/REPO", "cool_bug-fixes") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -623,8 +681,8 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { }, }) - output, err := RunCommand(`pr create`) - eq(t, err, nil) + output, err := runCommand(http, nil, "cool_bug-fixes", true, ``) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { @@ -652,10 +710,9 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { } func TestPRCreate_survey_defaults_monocommit(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) + http.Register(httpmock.GraphQL(`query RepositoryNetwork\b`), httpmock.StringResponse(httpmock.RepoNetworkStubResponse("OWNER", "REPO", "master", "WRITE"))) http.Register(httpmock.GraphQL(`query RepositoryFindFork\b`), httpmock.StringResponse(` { "data": { "repository": { "forks": { "nodes": [ @@ -708,15 +765,15 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { }, }) - output, err := RunCommand(`pr create`) + output, err := runCommand(http, nil, "feature", true, ``) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } func TestPRCreate_survey_autofill_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -744,8 +801,8 @@ func TestPRCreate_survey_autofill_nontty(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser open - output, err := RunCommand(`pr create -f`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", false, `-f`) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { @@ -775,9 +832,9 @@ func TestPRCreate_survey_autofill_nontty(t *testing.T) { } func TestPRCreate_survey_autofill(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -805,8 +862,8 @@ func TestPRCreate_survey_autofill(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser open - output, err := RunCommand(`pr create -f`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", true, `-f`) + require.NoError(t, err) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { @@ -834,9 +891,9 @@ func TestPRCreate_survey_autofill(t *testing.T) { } func TestPRCreate_defaults_error_autofill(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") cs, cmdTeardown := test.InitCmdStubber() @@ -847,15 +904,15 @@ func TestPRCreate_defaults_error_autofill(t *testing.T) { cs.Stub("") // git status cs.Stub("") // git log - _, err := RunCommand("pr create -f") + _, err := runCommand(http, nil, "feature", true, "-f") eq(t, err.Error(), "could not compute title or body defaults: could not find any commits between origin/master and feature") } func TestPRCreate_defaults_error_web(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") cs, cmdTeardown := test.InitCmdStubber() @@ -866,15 +923,15 @@ func TestPRCreate_defaults_error_web(t *testing.T) { cs.Stub("") // git status cs.Stub("") // git log - _, err := RunCommand("pr create -w") + _, err := runCommand(http, nil, "feature", true, "-w") eq(t, err.Error(), "could not compute title or body defaults: could not find any commits between origin/master and feature") } func TestPRCreate_defaults_error_interactive(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "forks": { "nodes": [ @@ -917,8 +974,8 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { }, }) - output, err := RunCommand(`pr create`) - eq(t, err, nil) + output, err := runCommand(http, nil, "feature", true, ``) + require.NoError(t, err) stderr := string(output.Stderr()) eq(t, strings.Contains(stderr, "warning: could not compute title or body defaults: could not find any commits"), true) diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index c38e7c78b..3d9d32083 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -31,7 +31,6 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman opts := &DiffOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -41,6 +40,9 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman Short: "View changes in a pull request", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + if len(args) > 0 { opts.SelectorArg = args[0] } diff --git a/test/fixtures/prList.json b/pkg/cmd/pr/list/fixtures/prList.json similarity index 100% rename from test/fixtures/prList.json rename to pkg/cmd/pr/list/fixtures/prList.json diff --git a/test/fixtures/prListWithDuplicates.json b/pkg/cmd/pr/list/fixtures/prListWithDuplicates.json similarity index 100% rename from test/fixtures/prListWithDuplicates.json rename to pkg/cmd/pr/list/fixtures/prListWithDuplicates.json diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go new file mode 100644 index 000000000..fd1ebb2b9 --- /dev/null +++ b/pkg/cmd/pr/list/list.go @@ -0,0 +1,170 @@ +package list + +import ( + "fmt" + "net/http" + "strconv" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/text" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type ListOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + + WebMode bool + LimitResults int + State string + BaseBranch string + Labels []string + Assignee string +} + +func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { + opts := &ListOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + } + + cmd := &cobra.Command{ + Use: "list", + Short: "List and filter pull requests in this repository", + Example: heredoc.Doc(` + $ gh pr list --limit 999 + $ gh pr list --state closed + $ gh pr list --label "priority 1" --label "bug" + $ gh pr list --web + `), + Args: cmdutil.NoArgsQuoteReminder, + RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + + if opts.LimitResults < 1 { + return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} + } + + if runF != nil { + return runF(opts) + } + return listRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open the browser to list the pull requests") + cmd.Flags().IntVarP(&opts.LimitResults, "limit", "L", 30, "Maximum number of items to fetch") + cmd.Flags().StringVarP(&opts.State, "state", "s", "open", "Filter by state: {open|closed|merged|all}") + cmd.Flags().StringVarP(&opts.BaseBranch, "base", "B", "", "Filter by base branch") + cmd.Flags().StringSliceVarP(&opts.Labels, "label", "l", nil, "Filter by labels") + cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") + + return cmd +} + +func listRun(opts *ListOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + if opts.WebMode { + prListURL := ghrepo.GenerateRepoURL(baseRepo, "pulls") + openURL, err := shared.ListURLWithQuery(prListURL, shared.FilterOptions{ + Entity: "pr", + State: opts.State, + Assignee: opts.Assignee, + Labels: opts.Labels, + BaseBranch: opts.BaseBranch, + }) + if err != nil { + return err + } + + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) + } + return utils.OpenInBrowser(openURL) + } + + var graphqlState []string + switch opts.State { + case "open": + graphqlState = []string{"OPEN"} + case "closed": + graphqlState = []string{"CLOSED", "MERGED"} + case "merged": + graphqlState = []string{"MERGED"} + case "all": + graphqlState = []string{"OPEN", "CLOSED", "MERGED"} + default: + return fmt.Errorf("invalid state: %s", opts.State) + } + + params := map[string]interface{}{ + "state": graphqlState, + } + if len(opts.Labels) > 0 { + params["labels"] = opts.Labels + } + if opts.BaseBranch != "" { + params["baseBranch"] = opts.BaseBranch + } + if opts.Assignee != "" { + params["assignee"] = opts.Assignee + } + + listResult, err := api.PullRequestList(apiClient, baseRepo, params, opts.LimitResults) + if err != nil { + return err + } + + if opts.IO.IsStdoutTTY() { + hasFilters := opts.State != "open" || len(opts.Labels) > 0 || opts.BaseBranch != "" || opts.Assignee != "" + title := shared.ListHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, hasFilters) + fmt.Fprintf(opts.IO.ErrOut, "\n%s\n\n", title) + } + + table := utils.NewTablePrinter(opts.IO) + for _, pr := range listResult.PullRequests { + prNum := strconv.Itoa(pr.Number) + if table.IsTTY() { + prNum = "#" + prNum + } + table.AddField(prNum, nil, shared.ColorFuncForPR(pr)) + table.AddField(text.ReplaceExcessiveWhitespace(pr.Title), nil, nil) + table.AddField(pr.HeadLabel(), nil, utils.Cyan) + if !table.IsTTY() { + table.AddField(prStateWithDraft(&pr), nil, nil) + } + table.EndRow() + } + err = table.Render() + if err != nil { + return err + } + + return nil +} + +func prStateWithDraft(pr *api.PullRequest) string { + if pr.IsDraft && pr.State == "OPEN" { + return "DRAFT" + } + + return pr.State +} diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go new file mode 100644 index 000000000..5faf42b57 --- /dev/null +++ b/pkg/cmd/pr/list/list_test.go @@ -0,0 +1,246 @@ +package list + +import ( + "bytes" + "io/ioutil" + "net/http" + "os/exec" + "reflect" + "regexp" + "strings" + "testing" + + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/test" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func eq(t *testing.T, got interface{}, expected interface{}) { + t.Helper() + if !reflect.DeepEqual(got, expected) { + t.Errorf("expected: %v, got: %v", expected, got) + } +} +func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + cmd := NewCmdList(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func initFakeHTTP() *httpmock.Registry { + return &httpmock.Registry{} +} + +func TestPRList(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("./fixtures/prList.json")) + + output, err := runCommand(http, true, "") + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, ` +Showing 3 of 3 open pull requests in OWNER/REPO + +`, output.Stderr()) + + lines := strings.Split(output.String(), "\n") + res := []*regexp.Regexp{ + regexp.MustCompile(`#32.*New feature.*feature`), + regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), + regexp.MustCompile(`#28.*Improve documentation.*docs`), + } + + for i, r := range res { + if !r.MatchString(lines[i]) { + t.Errorf("%s did not match %s", lines[i], r) + } + } +} + +func TestPRList_nontty(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("./fixtures/prList.json")) + + output, err := runCommand(http, false, "") + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, "", output.Stderr()) + + assert.Equal(t, `32 New feature feature DRAFT +29 Fixed bad bug hubot:bug-fix OPEN +28 Improve documentation docs MERGED +`, output.String()) +} + +func TestPRList_filtering(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestList\b`), + httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { + assert.Equal(t, []interface{}{"OPEN", "CLOSED", "MERGED"}, params["state"].([]interface{})) + assert.Equal(t, []interface{}{"one", "two", "three"}, params["labels"].([]interface{})) + })) + + output, err := runCommand(http, true, `-s all -l one,two -l three`) + if err != nil { + t.Fatal(err) + } + + eq(t, output.String(), "") + eq(t, output.Stderr(), ` +No pull requests match your search in OWNER/REPO + +`) +} + +func TestPRList_filteringRemoveDuplicate(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestList\b`), + httpmock.FileResponse("./fixtures/prListWithDuplicates.json")) + + output, err := runCommand(http, true, "-l one,two") + if err != nil { + t.Fatal(err) + } + + lines := strings.Split(output.String(), "\n") + + res := []*regexp.Regexp{ + regexp.MustCompile(`#32.*New feature.*feature`), + regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), + regexp.MustCompile(`#28.*Improve documentation.*docs`), + } + + for i, r := range res { + if !r.MatchString(lines[i]) { + t.Errorf("%s did not match %s", lines[i], r) + } + } +} + +func TestPRList_filteringClosed(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestList\b`), + httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { + assert.Equal(t, []interface{}{"CLOSED", "MERGED"}, params["state"].([]interface{})) + })) + + _, err := runCommand(http, true, `-s closed`) + if err != nil { + t.Fatal(err) + } +} + +func TestPRList_filteringAssignee(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestList\b`), + httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { + assert.Equal(t, `repo:OWNER/REPO assignee:hubot is:pr sort:created-desc is:merged label:"needs tests" base:"develop"`, params["q"].(string)) + })) + + _, err := runCommand(http, true, `-s merged -l "needs tests" -a hubot -B develop`) + if err != nil { + t.Fatal(err) + } +} + +func TestPRList_filteringAssigneeLabels(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + _, err := runCommand(http, true, `-l one,two -a hubot`) + if err == nil && err.Error() != "multiple labels with --assignee are not supported" { + t.Fatal(err) + } +} + +func TestPRList_withInvalidLimitFlag(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + _, err := runCommand(http, true, `--limit=0`) + if err == nil && err.Error() != "invalid limit: 0" { + t.Errorf("error running command `issue list`: %v", err) + } +} + +func TestPRList_web(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + var seenCmd *exec.Cmd + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { + seenCmd = cmd + return &test.OutputStub{} + }) + defer restoreCmd() + + output, err := runCommand(http, true, "--web -a peter -l bug -l docs -L 10 -s merged -B trunk") + if err != nil { + t.Errorf("error running command `pr list` with `--web` flag: %v", err) + } + + expectedURL := "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk" + + eq(t, output.String(), "") + eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/pulls in your browser.\n") + + if seenCmd == nil { + t.Fatal("expected a command to run") + } + url := seenCmd.Args[len(seenCmd.Args)-1] + eq(t, url, expectedURL) +} diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 00aa25695..b3c20b208 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -40,7 +40,6 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -62,6 +61,9 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + if len(args) > 0 { opts.SelectorArg = args[0] } diff --git a/pkg/cmd/pr/pr.go b/pkg/cmd/pr/pr.go new file mode 100644 index 000000000..599b01758 --- /dev/null +++ b/pkg/cmd/pr/pr.go @@ -0,0 +1,66 @@ +package pr + +import ( + "github.com/cli/cli/internal/ghrepo" + cmdCheckout "github.com/cli/cli/pkg/cmd/pr/checkout" + cmdClose "github.com/cli/cli/pkg/cmd/pr/close" + cmdCreate "github.com/cli/cli/pkg/cmd/pr/create" + cmdDiff "github.com/cli/cli/pkg/cmd/pr/diff" + cmdList "github.com/cli/cli/pkg/cmd/pr/list" + cmdMerge "github.com/cli/cli/pkg/cmd/pr/merge" + cmdReady "github.com/cli/cli/pkg/cmd/pr/ready" + cmdReopen "github.com/cli/cli/pkg/cmd/pr/reopen" + cmdReview "github.com/cli/cli/pkg/cmd/pr/review" + cmdStatus "github.com/cli/cli/pkg/cmd/pr/status" + cmdView "github.com/cli/cli/pkg/cmd/pr/view" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/pkg/cmdutil" + "github.com/spf13/cobra" +) + +func NewCmdPR(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "pr ", + Short: "Manage pull requests", + Long: "Work with GitHub pull requests", + Example: heredoc.Doc(` + $ gh pr checkout 353 + $ gh pr create --fill + $ gh pr view --web + `), + Annotations: map[string]string{ + "IsCore": "true", + "help:arguments": heredoc.Doc(` + A pull request can be supplied as argument in any of the following formats: + - by number, e.g. "123"; + - by URL, e.g. "https://github.com/OWNER/REPO/pull/123"; or + - by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1". + `), + }, + PersistentPreRun: func(cmd *cobra.Command, args []string) { + if repo, _ := cmd.Flags().GetString("repo"); repo != "" { + // NOTE: this mutates the factory + f.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.FromFullName(repo) + } + } + }, + } + + cmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") + + cmd.AddCommand(cmdCheckout.NewCmdCheckout(f, nil)) + cmd.AddCommand(cmdClose.NewCmdClose(f, nil)) + cmd.AddCommand(cmdCreate.NewCmdCreate(f, nil)) + cmd.AddCommand(cmdDiff.NewCmdDiff(f, nil)) + cmd.AddCommand(cmdList.NewCmdList(f, nil)) + cmd.AddCommand(cmdMerge.NewCmdMerge(f, nil)) + cmd.AddCommand(cmdReady.NewCmdReady(f, nil)) + cmd.AddCommand(cmdReopen.NewCmdReopen(f, nil)) + cmd.AddCommand(cmdReview.NewCmdReview(f, nil)) + cmd.AddCommand(cmdStatus.NewCmdStatus(f, nil)) + cmd.AddCommand(cmdView.NewCmdView(f, nil)) + + return cmd +} diff --git a/pkg/cmd/pr/ready/ready.go b/pkg/cmd/pr/ready/ready.go new file mode 100644 index 000000000..c5a248f19 --- /dev/null +++ b/pkg/cmd/pr/ready/ready.go @@ -0,0 +1,88 @@ +package ready + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type ReadyOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Remotes func() (context.Remotes, error) + Branch func() (string, error) + + SelectorArg string +} + +func NewCmdReady(f *cmdutil.Factory, runF func(*ReadyOptions) error) *cobra.Command { + opts := &ReadyOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + Remotes: f.Remotes, + Branch: f.Branch, + } + + cmd := &cobra.Command{ + Use: "ready [ | | ]", + Short: "Mark a pull request as ready for review", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return readyRun(opts) + }, + } + + return cmd +} + +func readyRun(opts *ReadyOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + if err != nil { + return err + } + + if pr.Closed { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is closed. Only draft pull requests can be marked as \"ready for review\"", utils.Red("!"), pr.Number) + return cmdutil.SilentError + } else if !pr.IsDraft { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is already \"ready for review\"\n", utils.Yellow("!"), pr.Number) + return nil + } + + err = api.PullRequestReady(apiClient, baseRepo, pr) + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is marked as \"ready for review\"\n", utils.Green("✔"), pr.Number) + + return nil +} diff --git a/pkg/cmd/pr/ready/ready_test.go b/pkg/cmd/pr/ready/ready_test.go new file mode 100644 index 000000000..8f5df5832 --- /dev/null +++ b/pkg/cmd/pr/ready/ready_test.go @@ -0,0 +1,135 @@ +package ready + +import ( + "bytes" + "io/ioutil" + "net/http" + "regexp" + "testing" + + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/test" + "github.com/google/shlex" +) + +func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Remotes: func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Branch: func() (string, error) { + return "main", nil + }, + } + + cmd := NewCmdReady(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPRReady(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 444, "closed": false, "isDraft": true} + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := runCommand(http, true, "444") + if err != nil { + t.Fatalf("error running command `pr ready`: %v", err) + } + + r := regexp.MustCompile(`Pull request #444 is marked as "ready for review"`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPRReady_alreadyReady(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 445, "closed": false, "isDraft": false} + } } } + `)) + + output, err := runCommand(http, true, "445") + if err != nil { + t.Fatalf("error running command `pr ready`: %v", err) + } + + r := regexp.MustCompile(`Pull request #445 is already "ready for review"`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPRReady_closed(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 446, "closed": true, "isDraft": true} + } } } + `)) + + output, err := runCommand(http, true, "446") + if err == nil { + t.Fatalf("expected an error running command `pr ready` on a review that is closed!: %v", err) + } + + r := regexp.MustCompile(`Pull request #446 is closed. Only draft pull requests can be marked as "ready for review"`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} diff --git a/pkg/cmd/pr/reopen/reopen.go b/pkg/cmd/pr/reopen/reopen.go new file mode 100644 index 000000000..78f4a36ec --- /dev/null +++ b/pkg/cmd/pr/reopen/reopen.go @@ -0,0 +1,85 @@ +package reopen + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type ReopenOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + + SelectorArg string +} + +func NewCmdReopen(f *cmdutil.Factory, runF func(*ReopenOptions) error) *cobra.Command { + opts := &ReopenOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + } + + cmd := &cobra.Command{ + Use: "reopen { | | }", + Short: "Reopen a pull request", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return reopenRun(opts) + }, + } + + return cmd +} + +func reopenRun(opts *ReopenOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, nil, nil, opts.SelectorArg) + if err != nil { + return err + } + + if pr.State == "MERGED" { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be reopened because it was already merged", utils.Red("!"), pr.Number, pr.Title) + return cmdutil.SilentError + } + + if !pr.Closed { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already open\n", utils.Yellow("!"), pr.Number, pr.Title) + return nil + } + + err = api.PullRequestReopen(apiClient, baseRepo, pr) + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Reopened pull request #%d (%s)\n", utils.Green("✔"), pr.Number, pr.Title) + + return nil +} diff --git a/pkg/cmd/pr/reopen/reopen_test.go b/pkg/cmd/pr/reopen/reopen_test.go new file mode 100644 index 000000000..24dfa488f --- /dev/null +++ b/pkg/cmd/pr/reopen/reopen_test.go @@ -0,0 +1,123 @@ +package reopen + +import ( + "bytes" + "io/ioutil" + "net/http" + "regexp" + "testing" + + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/test" + "github.com/google/shlex" +) + +func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + cmd := NewCmdReopen(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPRReopen(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "title": "The title of the PR", "closed": true} + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := runCommand(http, true, "666") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Reopened pull request #666 \(The title of the PR\)`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPRReopen_alreadyOpen(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "title": "The title of the PR", "closed": false} + } } } + `)) + + output, err := runCommand(http, true, "666") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) is already open`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPRReopen_alreadyMerged(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "title": "The title of the PR", "closed": true, "state": "MERGED"} + } } } + `)) + + output, err := runCommand(http, true, "666") + if err == nil { + t.Fatalf("expected an error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) can't be reopened because it was already merged`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} diff --git a/pkg/cmd/pr/review/review.go b/pkg/cmd/pr/review/review.go index 61bfcea59..dba5dff10 100644 --- a/pkg/cmd/pr/review/review.go +++ b/pkg/cmd/pr/review/review.go @@ -40,7 +40,6 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -68,6 +67,9 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + if len(args) > 0 { opts.SelectorArg = args[0] } diff --git a/pkg/cmd/pr/shared/display.go b/pkg/cmd/pr/shared/display.go index f43b7fa67..31e7228b6 100644 --- a/pkg/cmd/pr/shared/display.go +++ b/pkg/cmd/pr/shared/display.go @@ -45,3 +45,22 @@ func PrintHeader(w io.Writer, s string) { func PrintMessage(w io.Writer, s string) { fmt.Fprintln(w, utils.Gray(s)) } + +func ListHeader(repoName string, itemName string, matchCount int, totalMatchCount int, hasFilters bool) string { + if totalMatchCount == 0 { + if hasFilters { + return fmt.Sprintf("No %ss match your search in %s", itemName, repoName) + } + return fmt.Sprintf("There are no open %ss in %s", itemName, repoName) + } + + if hasFilters { + matchVerb := "match" + if totalMatchCount == 1 { + matchVerb = "matches" + } + return fmt.Sprintf("Showing %d of %s in %s that %s your search", matchCount, utils.Pluralize(totalMatchCount, itemName), repoName, matchVerb) + } + + return fmt.Sprintf("Showing %d of %s in %s", matchCount, utils.Pluralize(totalMatchCount, fmt.Sprintf("open %s", itemName)), repoName) +} diff --git a/pkg/cmd/pr/shared/display_test.go b/pkg/cmd/pr/shared/display_test.go new file mode 100644 index 000000000..d958d2265 --- /dev/null +++ b/pkg/cmd/pr/shared/display_test.go @@ -0,0 +1,114 @@ +package shared + +import "testing" + +func Test_listHeader(t *testing.T) { + type args struct { + repoName string + itemName string + matchCount int + totalMatchCount int + hasFilters bool + } + tests := []struct { + name string + args args + want string + }{ + { + name: "no results", + args: args{ + repoName: "REPO", + itemName: "table", + matchCount: 0, + totalMatchCount: 0, + hasFilters: false, + }, + want: "There are no open tables in REPO", + }, + { + name: "no matches after filters", + args: args{ + repoName: "REPO", + itemName: "Luftballon", + matchCount: 0, + totalMatchCount: 0, + hasFilters: true, + }, + want: "No Luftballons match your search in REPO", + }, + { + name: "one result", + args: args{ + repoName: "REPO", + itemName: "genie", + matchCount: 1, + totalMatchCount: 23, + hasFilters: false, + }, + want: "Showing 1 of 23 open genies in REPO", + }, + { + name: "one result after filters", + args: args{ + repoName: "REPO", + itemName: "tiny cup", + matchCount: 1, + totalMatchCount: 23, + hasFilters: true, + }, + want: "Showing 1 of 23 tiny cups in REPO that match your search", + }, + { + name: "one result in total", + args: args{ + repoName: "REPO", + itemName: "chip", + matchCount: 1, + totalMatchCount: 1, + hasFilters: false, + }, + want: "Showing 1 of 1 open chip in REPO", + }, + { + name: "one result in total after filters", + args: args{ + repoName: "REPO", + itemName: "spicy noodle", + matchCount: 1, + totalMatchCount: 1, + hasFilters: true, + }, + want: "Showing 1 of 1 spicy noodle in REPO that matches your search", + }, + { + name: "multiple results", + args: args{ + repoName: "REPO", + itemName: "plant", + matchCount: 4, + totalMatchCount: 23, + hasFilters: false, + }, + want: "Showing 4 of 23 open plants in REPO", + }, + { + name: "multiple results after filters", + args: args{ + repoName: "REPO", + itemName: "boomerang", + matchCount: 4, + totalMatchCount: 23, + hasFilters: true, + }, + want: "Showing 4 of 23 boomerangs in REPO that match your search", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ListHeader(tt.args.repoName, tt.args.itemName, tt.args.matchCount, tt.args.totalMatchCount, tt.args.hasFilters); got != tt.want { + t.Errorf("listHeader() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go new file mode 100644 index 000000000..616b4b358 --- /dev/null +++ b/pkg/cmd/pr/shared/params.go @@ -0,0 +1,165 @@ +package shared + +import ( + "fmt" + "net/url" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghrepo" +) + +func WithPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, projects []string, milestones []string) (string, error) { + u, err := url.Parse(baseURL) + if err != nil { + return "", err + } + q := u.Query() + if title != "" { + q.Set("title", title) + } + if body != "" { + q.Set("body", body) + } + if len(assignees) > 0 { + q.Set("assignees", strings.Join(assignees, ",")) + } + if len(labels) > 0 { + q.Set("labels", strings.Join(labels, ",")) + } + if len(projects) > 0 { + q.Set("projects", strings.Join(projects, ",")) + } + if len(milestones) > 0 { + q.Set("milestone", milestones[0]) + } + u.RawQuery = q.Encode() + return u.String(), nil +} + +func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *IssueMetadataState) error { + if !tb.HasMetadata() { + return nil + } + + if tb.MetadataResult == nil { + resolveInput := api.RepoResolveInput{ + Reviewers: tb.Reviewers, + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: tb.Milestones, + } + + var err error + tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) + if err != nil { + return err + } + } + + assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) + if err != nil { + return fmt.Errorf("could not assign user: %w", err) + } + params["assigneeIds"] = assigneeIDs + + labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) + if err != nil { + return fmt.Errorf("could not add label: %w", err) + } + params["labelIds"] = labelIDs + + projectIDs, err := tb.MetadataResult.ProjectsToIDs(tb.Projects) + if err != nil { + return fmt.Errorf("could not add to project: %w", err) + } + params["projectIds"] = projectIDs + + if len(tb.Milestones) > 0 { + milestoneID, err := tb.MetadataResult.MilestoneToID(tb.Milestones[0]) + if err != nil { + return fmt.Errorf("could not add to milestone '%s': %w", tb.Milestones[0], err) + } + params["milestoneId"] = milestoneID + } + + if len(tb.Reviewers) == 0 { + return nil + } + + var userReviewers []string + var teamReviewers []string + for _, r := range tb.Reviewers { + if strings.ContainsRune(r, '/') { + teamReviewers = append(teamReviewers, r) + } else { + userReviewers = append(userReviewers, r) + } + } + + userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["userReviewerIds"] = userReviewerIDs + + teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["teamReviewerIds"] = teamReviewerIDs + + return nil +} + +type FilterOptions struct { + Entity string + State string + Assignee string + Labels []string + Author string + BaseBranch string + Mention string + Milestone string +} + +func ListURLWithQuery(listURL string, options FilterOptions) (string, error) { + u, err := url.Parse(listURL) + if err != nil { + return "", err + } + query := fmt.Sprintf("is:%s ", options.Entity) + if options.State != "all" { + query += fmt.Sprintf("is:%s ", options.State) + } + if options.Assignee != "" { + query += fmt.Sprintf("assignee:%s ", options.Assignee) + } + for _, label := range options.Labels { + query += fmt.Sprintf("label:%s ", quoteValueForQuery(label)) + } + if options.Author != "" { + query += fmt.Sprintf("author:%s ", options.Author) + } + if options.BaseBranch != "" { + query += fmt.Sprintf("base:%s ", options.BaseBranch) + } + if options.Mention != "" { + query += fmt.Sprintf("mentions:%s ", options.Mention) + } + if options.Milestone != "" { + query += fmt.Sprintf("milestone:%s ", quoteValueForQuery(options.Milestone)) + } + q := u.Query() + q.Set("q", strings.TrimSuffix(query, " ")) + u.RawQuery = q.Encode() + return u.String(), nil +} + +func quoteValueForQuery(v string) string { + if strings.ContainsAny(v, " \"\t\r\n") { + return fmt.Sprintf("%q", v) + } + return v +} diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go new file mode 100644 index 000000000..177ae0148 --- /dev/null +++ b/pkg/cmd/pr/shared/params_test.go @@ -0,0 +1,71 @@ +package shared + +import "testing" + +func Test_listURLWithQuery(t *testing.T) { + type args struct { + listURL string + options FilterOptions + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "blank", + args: args{ + listURL: "https://example.com/path?a=b", + options: FilterOptions{ + Entity: "issue", + State: "open", + }, + }, + want: "https://example.com/path?a=b&q=is%3Aissue+is%3Aopen", + wantErr: false, + }, + { + name: "all", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "issue", + State: "open", + Assignee: "bo", + Author: "ka", + BaseBranch: "trunk", + Mention: "nu", + }, + }, + want: "https://example.com/path?q=is%3Aissue+is%3Aopen+assignee%3Abo+author%3Aka+base%3Atrunk+mentions%3Anu", + wantErr: false, + }, + { + name: "spaces in values", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "pr", + State: "open", + Labels: []string{"docs", "help wanted"}, + Milestone: `Codename "What Was Missing"`, + }, + }, + want: "https://example.com/path?q=is%3Apr+is%3Aopen+label%3Adocs+label%3A%22help+wanted%22+milestone%3A%22Codename+%5C%22What+Was+Missing%5C%22%22", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ListURLWithQuery(tt.args.listURL, tt.args.options) + if (err != nil) != tt.wantErr { + t.Errorf("listURLWithQuery() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("listURLWithQuery() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/command/title_body_survey.go b/pkg/cmd/pr/shared/title_body_survey.go similarity index 88% rename from command/title_body_survey.go rename to pkg/cmd/pr/shared/title_body_survey.go index 94ec89914..3afeba803 100644 --- a/command/title_body_survey.go +++ b/pkg/cmd/pr/shared/title_body_survey.go @@ -1,4 +1,4 @@ -package command +package shared import ( "fmt" @@ -7,21 +7,26 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/githubtemplate" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/pkg/surveyext" "github.com/cli/cli/utils" - "github.com/spf13/cobra" ) +type Defaults struct { + Title string + Body string +} + type Action int type metadataStateType int const ( - issueMetadata metadataStateType = iota - prMetadata + IssueMetadata metadataStateType = iota + PRMetadata ) -type issueMetadataState struct { +type IssueMetadataState struct { Type metadataStateType Body string @@ -38,7 +43,7 @@ type issueMetadataState struct { MetadataResult *api.RepoMetadataResult } -func (tb *issueMetadataState) HasMetadata() bool { +func (tb *IssueMetadataState) HasMetadata() bool { return len(tb.Reviewers) > 0 || len(tb.Assignees) > 0 || len(tb.Labels) > 0 || @@ -112,9 +117,9 @@ func selectTemplate(nonLegacyTemplatePaths []string, legacyTemplatePath *string, for _, p := range nonLegacyTemplatePaths { templateNames = append(templateNames, githubtemplate.ExtractName(p)) } - if metadataType == issueMetadata { + if metadataType == IssueMetadata { templateNames = append(templateNames, "Open a blank issue") - } else if metadataType == prMetadata { + } else if metadataType == PRMetadata { templateNames = append(templateNames, "Open a blank pull request") } @@ -143,12 +148,8 @@ func selectTemplate(nonLegacyTemplatePaths []string, legacyTemplatePath *string, return string(templateContents), nil } -func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs defaults, nonLegacyTemplatePaths []string, legacyTemplatePath *string, allowReviewers, allowMetadata bool) error { - editorCommand, err := determineEditor(cmd) - if err != nil { - return err - } - +// FIXME: this command has too many parameters and responsibilities +func TitleBodySurvey(io *iostreams.IOStreams, editorCommand string, issueState *IssueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs Defaults, nonLegacyTemplatePaths []string, legacyTemplatePath *string, allowReviewers, allowMetadata bool) error { issueState.Title = defs.Title templateContents := "" @@ -198,7 +199,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie qs = append(qs, bodyQuestion) } - err = prompt.SurveyAsk(qs, issueState) + err := prompt.SurveyAsk(qs, issueState) if err != nil { return fmt.Errorf("could not prompt: %w", err) } @@ -249,7 +250,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie Projects: isChosen("Projects"), Milestones: isChosen("Milestone"), } - s := utils.Spinner(cmd.OutOrStderr()) + s := utils.Spinner(io.ErrOut) utils.StartSpinner(s) issueState.MetadataResult, err = api.RepoMetadata(apiClient, repo, metadataInput) utils.StopSpinner(s) @@ -297,7 +298,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie }, }) } else { - cmd.PrintErrln("warning: no available reviewers") + fmt.Fprintln(io.ErrOut, "warning: no available reviewers") } } if isChosen("Assignees") { @@ -311,7 +312,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie }, }) } else { - cmd.PrintErrln("warning: no assignable users") + fmt.Fprintln(io.ErrOut, "warning: no assignable users") } } if isChosen("Labels") { @@ -325,7 +326,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie }, }) } else { - cmd.PrintErrln("warning: no labels in the repository") + fmt.Fprintln(io.ErrOut, "warning: no labels in the repository") } } if isChosen("Projects") { @@ -339,7 +340,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie }, }) } else { - cmd.PrintErrln("warning: no projects to choose from") + fmt.Fprintln(io.ErrOut, "warning: no projects to choose from") } } if isChosen("Milestone") { @@ -357,7 +358,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie }, }) } else { - cmd.PrintErrln("warning: no milestones in the repository") + fmt.Fprintln(io.ErrOut, "warning: no milestones in the repository") } } values := metadataValues{} diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 8708dae65..dffd2b412 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -38,7 +38,6 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -48,6 +47,8 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co Short: "Show status of relevant pull requests", Args: cmdutil.NoArgsQuoteReminder, RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo opts.HasRepoOverride = cmd.Flags().Changed("repo") if runF != nil { diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index b93bfcd1d..169d989f1 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -36,7 +36,6 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - BaseRepo: f.BaseRepo, Remotes: f.Remotes, Branch: f.Branch, } @@ -54,6 +53,9 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + if len(args) > 0 { opts.SelectorArg = args[0] } diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index a1ffdad32..cf69ecd8c 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -2,12 +2,17 @@ package iostreams import ( "bytes" + "fmt" "io" "io/ioutil" "os" + "os/exec" + "strconv" + "strings" "github.com/mattn/go-colorable" "github.com/mattn/go-isatty" + "golang.org/x/crypto/ssh/terminal" ) type IOStreams struct { @@ -74,6 +79,29 @@ func (s *IOStreams) IsStderrTTY() bool { return false } +func (s *IOStreams) TerminalWidth() int { + defaultWidth := 80 + if s.stdoutTTYOverride { + return defaultWidth + } + + if w, _, err := terminalSize(s.Out); err == nil { + return w + } + + if isCygwinTerminal(s.Out) { + tputCmd := exec.Command("tput", "cols") + tputCmd.Stdin = os.Stdin + if out, err := tputCmd.Output(); err == nil { + if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { + return w + } + } + } + + return defaultWidth +} + func System() *IOStreams { var out io.Writer = os.Stdout var colorEnabled bool @@ -104,3 +132,17 @@ func Test() (*IOStreams, *bytes.Buffer, *bytes.Buffer, *bytes.Buffer) { func isTerminal(f *os.File) bool { return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) } + +func isCygwinTerminal(w io.Writer) bool { + if f, isFile := w.(*os.File); isFile { + return isatty.IsCygwinTerminal(f.Fd()) + } + return false +} + +func terminalSize(w io.Writer) (int, int, error) { + if f, isFile := w.(*os.File); isFile { + return terminal.GetSize(int(f.Fd())) + } + return 0, 0, fmt.Errorf("%v is not a file", w) +} diff --git a/utils/table_printer.go b/utils/table_printer.go index 823a4b533..3047e2d81 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -3,11 +3,9 @@ package utils import ( "fmt" "io" - "os" - "os/exec" - "strconv" "strings" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/text" ) @@ -18,28 +16,15 @@ type TablePrinter interface { Render() error } -func NewTablePrinter(w io.Writer) TablePrinter { - if IsTerminal(w) { - isCygwin := IsCygwinTerminal(w) - ttyWidth := 80 - if termWidth, _, err := TerminalSize(w); err == nil { - ttyWidth = termWidth - } else if isCygwin { - tputCmd := exec.Command("tput", "cols") - tputCmd.Stdin = os.Stdin - if out, err := tputCmd.Output(); err == nil { - if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { - ttyWidth = w - } - } - } +func NewTablePrinter(io *iostreams.IOStreams) TablePrinter { + if io.IsStdoutTTY() { return &ttyTablePrinter{ - out: NewColorable(w), - maxWidth: ttyWidth, + out: io.Out, + maxWidth: io.TerminalWidth(), } } return &tsvTablePrinter{ - out: w, + out: io.Out, } }