From 8f67ad44e07a7676e5b86bbd1a259651b84c004e Mon Sep 17 00:00:00 2001 From: Victor Hugo Date: Fri, 8 May 2020 15:04:02 -0400 Subject: [PATCH 01/49] add description default private for repo create --- command/repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/repo.go b/command/repo.go index a611643be..982a98595 100644 --- a/command/repo.go +++ b/command/repo.go @@ -28,7 +28,7 @@ func init() { repoCreateCmd.Flags().StringP("team", "t", "", "The name of the organization team to be granted access") repoCreateCmd.Flags().Bool("enable-issues", true, "Enable issues in the new repository") repoCreateCmd.Flags().Bool("enable-wiki", true, "Enable wiki in the new repository") - repoCreateCmd.Flags().Bool("public", false, "Make the new repository public") + repoCreateCmd.Flags().Bool("public", false, "Make the new repository public (default: private)") repoCmd.AddCommand(repoForkCmd) repoForkCmd.Flags().String("clone", "prompt", "Clone fork: {true|false|prompt}") From 92130d91ba30fe142b9a1fa677444fbcaea7bdcf Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Mon, 11 May 2020 14:08:55 -0400 Subject: [PATCH 02/49] Minimal manpage generation from cobra --- .gitignore | 2 ++ .goreleaser.yml | 4 ++++ Makefile | 8 ++++++++ cmd/gen-docs/main.go | 5 +++++ 4 files changed, 19 insertions(+) diff --git a/.gitignore b/.gitignore index 5ef399ba7..80891986e 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,5 @@ .DS_Store vendor/ + +manpage diff --git a/.goreleaser.yml b/.goreleaser.yml index 8fca69fb3..4ceeee571 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -35,6 +35,8 @@ archives: replacements: darwin: macOS format: tar.gz + files: + - manpage/gh*.1 - id: windows builds: [windows] <<: *archive_defaults @@ -76,6 +78,8 @@ nfpms: formats: - deb - rpm + files: + "./manpage/gh*.1": "/usr/share/man/man1" scoop: bucket: diff --git a/Makefile b/Makefile index f2b4805c8..9dc9aae50 100644 --- a/Makefile +++ b/Makefile @@ -44,3 +44,11 @@ endif git -C site commit -m '$(GITHUB_REF:refs/tags/v%=%)' index.html git -C site push .PHONY: site-publish + + +manpage: + mkdir -p $@ + +.PHONY: manpages +manpages: manpage + go run ./cmd/gen-docs ./manpage diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index 6607054a9..82efa58f9 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -24,6 +24,11 @@ func main() { if err != nil { fatal(err) } + + err = doc.GenManTree(command.RootCmd, nil, dir) + if err != nil { + fatal(err) + } } func filePrepender(filename string) string { From 6d57f2a91e569f99f825797f628d9156a02a1fc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 11 May 2020 20:35:39 +0200 Subject: [PATCH 03/49] Faster resolve GraphQL node IDs for assignees, reviewers, and labels --- api/queries_repo.go | 100 +++++++++++++++++++++++++++++++++++++++ api/queries_repo_test.go | 95 +++++++++++++++++++++++++++++++++++++ command/pr_create.go | 19 ++++---- pkg/httpmock/stub.go | 16 +++++++ 4 files changed, 222 insertions(+), 8 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index af578413d..6eb7088fc 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -580,6 +580,106 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return &result, err } +type RepoResolveInput struct { + Assignees []string + Reviewers []string + Labels []string + Projects []string + Milestones []string +} + +// RepoResolveMetadataIDs looks up GraphQL node IDs in bulk +func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoResolveInput) (*RepoMetadataResult, error) { + users := input.Assignees + hasUser := func(target string) bool { + for _, u := range users { + if strings.EqualFold(u, target) { + return true + } + } + return false + } + + var teams []string + for _, r := range input.Reviewers { + if i := strings.IndexRune(r, '/'); i > -1 { + teams = append(teams, r[i+1:]) + } else if !hasUser(r) { + users = append(users, r) + } + } + + // there is no way to look up projects nor milestones by name, so preload them all + mi := RepoMetadataInput{ + Projects: len(input.Projects) > 0, + Milestones: len(input.Milestones) > 0, + } + result, err := RepoMetadata(client, repo, mi) + if err != nil { + return result, err + } + if len(users) == 0 && len(teams) == 0 && len(input.Labels) == 0 { + return result, nil + } + + query := &bytes.Buffer{} + for i, u := range users { + fmt.Fprintf(query, "u%03d: user(login:%q){id,login}\n", i, u) + } + if len(input.Labels) > 0 { + fmt.Fprintf(query, "repository(owner:%q,name:%q){\n", repo.RepoOwner(), repo.RepoName()) + for i, l := range input.Labels { + fmt.Fprintf(query, "l%03d: label(name:%q){id,name}\n", i, l) + } + fmt.Fprint(query, "}\n") + } + if len(teams) > 0 { + fmt.Fprintf(query, "organization(login:%q){\n", repo.RepoOwner()) + for i, t := range teams { + fmt.Fprintf(query, "t%03d: team(slug:%q){id,slug}\n", i, t) + } + fmt.Fprint(query, "}\n") + } + + response := make(map[string]json.RawMessage) + err = client.GraphQL(query.String(), nil, &response) + if err != nil { + return result, err + } + + for key, v := range response { + switch key { + case "repository": + repoResponse := make(map[string]RepoLabel) + err := json.Unmarshal(v, &repoResponse) + if err != nil { + return result, err + } + for _, l := range repoResponse { + result.Labels = append(result.Labels, l) + } + case "organization": + orgResponse := make(map[string]OrgTeam) + err := json.Unmarshal(v, &orgResponse) + if err != nil { + return result, err + } + for _, t := range orgResponse { + result.Teams = append(result.Teams, t) + } + default: + user := RepoAssignee{} + err := json.Unmarshal(v, &user) + if err != nil { + return result, err + } + result.AssignableUsers = append(result.AssignableUsers, user) + } + } + + return result, nil +} + type RepoProject struct { ID string Name string diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index d8a75f4ff..6e36f4abc 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "testing" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" ) @@ -45,3 +46,97 @@ func Test_RepoCreate(t *testing.T) { t.Errorf("expected homepageUrl to be %q, got %q", "http://example.com", homepage) } } + +func Test_RepoResolveMetadataIDs(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + repo := ghrepo.FromFullName("OWNER/REPO") + input := RepoResolveInput{ + Assignees: []string{"monalisa", "hubot"}, + Reviewers: []string{"monalisa", "octocat", "OWNER/core", "/robots"}, + Labels: []string{"bug", "help wanted"}, + } + + expectedQuery := `u000: user(login:"monalisa"){id,login} +u001: user(login:"hubot"){id,login} +u002: user(login:"octocat"){id,login} +repository(owner:"OWNER",name:"REPO"){ +l000: label(name:"bug"){id,name} +l001: label(name:"help wanted"){id,name} +} +organization(login:"OWNER"){ +t000: team(slug:"core"){id,slug} +t001: team(slug:"robots"){id,slug} +} +` + responseJSON := ` + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "u001": { "login": "hubot", "id": "HUBOTID" }, + "u002": { "login": "octocat", "id": "OCTOID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "Help Wanted", "id": "HELPID" } + }, + "organization": { + "t000": { "slug": "core", "id": "COREID" }, + "t001": { "slug": "Robots", "id": "ROBOTID" } + } + } } + ` + + http.Register( + httpmock.MatchAny, + httpmock.GraphQLQuery(responseJSON, func(q string, _ map[string]interface{}) { + if q != expectedQuery { + t.Errorf("expected query %q, got %q", expectedQuery, q) + } + })) + + result, err := RepoResolveMetadataIDs(client, repo, input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedMemberIDs := []string{"MONAID", "HUBOTID", "OCTOID"} + memberIDs, err := result.MembersToIDs([]string{"monalisa", "hubot", "octocat"}) + if err != nil { + t.Errorf("error resolving members: %v", err) + } + if !sliceEqual(memberIDs, expectedMemberIDs) { + t.Errorf("expected members %v, got %v", expectedMemberIDs, memberIDs) + } + + expectedTeamIDs := []string{"COREID", "ROBOTID"} + teamIDs, err := result.TeamsToIDs([]string{"/core", "/robots"}) + if err != nil { + t.Errorf("error resolving teams: %v", err) + } + if !sliceEqual(teamIDs, expectedTeamIDs) { + t.Errorf("expected members %v, got %v", expectedTeamIDs, teamIDs) + } + + expectedLabelIDs := []string{"BUGID", "HELPID"} + labelIDs, err := result.LabelsToIDs([]string{"bug", "help wanted"}) + if err != nil { + t.Errorf("error resolving labels: %v", err) + } + if !sliceEqual(labelIDs, expectedLabelIDs) { + t.Errorf("expected members %v, got %v", expectedLabelIDs, labelIDs) + } +} + +func sliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + + for i := range a { + if a[i] != b[i] { + return false + } + } + + return true +} diff --git a/command/pr_create.go b/command/pr_create.go index 4ce58a681..e50855f4a 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -325,16 +325,19 @@ func prCreate(cmd *cobra.Command, _ []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - metadataInput := api.RepoMetadataInput{ - Reviewers: len(tb.Reviewers) > 0, - Assignees: len(tb.Assignees) > 0, - Labels: len(tb.Labels) > 0, - Projects: len(tb.Projects) > 0, - Milestones: tb.Milestone != "", + var milestones []string + if tb.Milestone != "" { + milestones = append(milestones, tb.Milestone) + } + resolveInput := api.RepoResolveInput{ + Reviewers: tb.Reviewers, + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: milestones, } - // TODO: for non-interactive mode, only translate given objects to GraphQL IDs - tb.MetadataResult, err = api.RepoMetadata(client, baseRepo, metadataInput) + tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) if err != nil { return err } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index b27ceea6c..48077ed4e 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -88,6 +88,22 @@ func GraphQLMutation(body string, cb func(map[string]interface{})) Responder { } } +func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responder { + return func(req *http.Request) (*http.Response, error) { + var bodyData struct { + Query string + Variables map[string]interface{} + } + err := decodeJSONBody(req, &bodyData) + if err != nil { + return nil, err + } + cb(bodyData.Query, bodyData.Variables) + + return httpResponse(200, bytes.NewBufferString(body)), nil + } +} + func httpResponse(status int, body io.Reader) *http.Response { return &http.Response{ StatusCode: status, From 00f23b8d865062b3dd4f34067b301f52cabac7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 15:48:59 +0200 Subject: [PATCH 04/49] Store milestones as slice for consistency with other metadata This is for code simplicity. Only 1 milestone per issue or PR is allowed, like before. --- command/issue.go | 26 ++++++++++++++------------ command/pr_create.go | 24 +++++++++++------------- command/title_body_survey.go | 28 +++++++++++++++++----------- 3 files changed, 42 insertions(+), 36 deletions(-) diff --git a/command/issue.go b/command/issue.go index e5cf24026..b5e44787c 100644 --- a/command/issue.go +++ b/command/issue.go @@ -381,9 +381,11 @@ func issueCreate(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("could not parse projects: %w", err) } - milestoneTitle, err := cmd.Flags().GetString("milestone") - if err != nil { + 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 isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { @@ -419,10 +421,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction tb := issueMetadataState{ - Assignees: assignees, - Labels: labelNames, - Projects: projectNames, - Milestone: milestoneTitle, + Assignees: assignees, + Labels: labelNames, + Projects: projectNames, + Milestones: milestoneTitles, } interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) @@ -475,7 +477,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { Assignees: len(tb.Assignees) > 0, Labels: len(tb.Labels) > 0, Projects: len(tb.Projects) > 0, - Milestones: tb.Milestone != "", + Milestones: len(tb.Milestones) > 0, } // TODO: for non-interactive mode, only translate given objects to GraphQL IDs @@ -485,7 +487,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } } - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestone) + err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) if err != nil { return err } @@ -504,7 +506,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames []string, milestoneTitle string) error { +func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames, milestoneTitles []string) error { assigneeIDs, err := metadata.MembersToIDs(assignees) if err != nil { return fmt.Errorf("could not assign user: %w", err) @@ -523,10 +525,10 @@ func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoM } params["projectIds"] = projectIDs - if milestoneTitle != "" { - milestoneID, err := metadata.MilestoneToID(milestoneTitle) + if len(milestoneTitles) > 0 { + milestoneID, err := metadata.MilestoneToID(milestoneTitles[0]) if err != nil { - return fmt.Errorf("could not add to milestone '%s': %w", milestoneTitle, err) + return fmt.Errorf("could not add to milestone '%s': %w", milestoneTitles[0], err) } params["milestoneId"] = milestoneID } diff --git a/command/pr_create.go b/command/pr_create.go index e50855f4a..9e096c0eb 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -140,9 +140,11 @@ func prCreate(cmd *cobra.Command, _ []string) error { if err != nil { return fmt.Errorf("could not parse projects: %w", err) } - milestoneTitle, err := cmd.Flags().GetString("milestone") - if err != nil { + 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) } baseTrackingBranch := baseBranch @@ -201,11 +203,11 @@ func prCreate(cmd *cobra.Command, _ []string) error { } tb := issueMetadataState{ - Reviewers: reviewers, - Assignees: assignees, - Labels: labelNames, - Projects: projectNames, - Milestone: milestoneTitle, + Reviewers: reviewers, + Assignees: assignees, + Labels: labelNames, + Projects: projectNames, + Milestones: milestoneTitles, } interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) @@ -325,16 +327,12 @@ func prCreate(cmd *cobra.Command, _ []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - var milestones []string - if tb.Milestone != "" { - milestones = append(milestones, tb.Milestone) - } resolveInput := api.RepoResolveInput{ Reviewers: tb.Reviewers, Assignees: tb.Assignees, Labels: tb.Labels, Projects: tb.Projects, - Milestones: milestones, + Milestones: tb.Milestones, } tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) @@ -343,7 +341,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestone) + err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) if err != nil { return err } diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 06ab14a09..f8200c218 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -20,12 +20,12 @@ type issueMetadataState struct { Title string Action Action - Metadata []string - Reviewers []string - Assignees []string - Labels []string - Projects []string - Milestone string + Metadata []string + Reviewers []string + Assignees []string + Labels []string + Projects []string + Milestones []string MetadataResult *api.RepoMetadataResult } @@ -35,7 +35,7 @@ func (tb *issueMetadataState) HasMetadata() bool { len(tb.Assignees) > 0 || len(tb.Labels) > 0 || len(tb.Projects) > 0 || - tb.Milestone != "" + len(tb.Milestones) > 0 } const ( @@ -43,6 +43,8 @@ const ( PreviewAction CancelAction MetadataAction + + noMilestone = "(none)" ) var SurveyAsk = func(qs []*survey.Question, response interface{}, opts ...survey.AskOpt) error { @@ -257,7 +259,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie for _, l := range issueState.MetadataResult.Projects { projects = append(projects, l.Name) } - milestones := []string{"(none)"} + milestones := []string{noMilestone} for _, m := range issueState.MetadataResult.Milestones { milestones = append(milestones, m.Title) } @@ -321,12 +323,16 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie } if isChosen("Milestone") { if len(milestones) > 1 { + var milestoneDefault interface{} + if len(issueState.Milestones) > 0 { + milestoneDefault = issueState.Milestones[0] + } mqs = append(mqs, &survey.Question{ Name: "milestone", Prompt: &survey.Select{ Message: "Milestone", Options: milestones, - Default: issueState.Milestone, + Default: milestoneDefault, }, }) } else { @@ -339,8 +345,8 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie return fmt.Errorf("could not prompt: %w", err) } - if issueState.Milestone == "(none)" { - issueState.Milestone = "" + if len(issueState.Milestones) > 0 && issueState.Milestones[0] == noMilestone { + issueState.Milestones = issueState.Milestones[0:0] } allowPreview = !issueState.HasMetadata() From e33706fbf49da6a2de1424775b9cc339858701b1 Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 12 May 2020 09:51:25 -0400 Subject: [PATCH 05/49] Add flags for man vs website to cmd/gen-docs --- Makefile | 4 ++-- cmd/gen-docs/main.go | 48 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 9dc9aae50..87ec9d8d5 100644 --- a/Makefile +++ b/Makefile @@ -28,7 +28,7 @@ site: site-docs: site git -C site pull git -C site rm 'manual/gh*.md' 2>/dev/null || true - go run ./cmd/gen-docs site/manual + go run ./cmd/gen-docs --website --doc-path site/manual for f in site/manual/gh*.md; do sed -i.bak -e '/^### SEE ALSO/,$$d' "$$f"; done rm -f site/manual/*.bak git -C site add 'manual/gh*.md' @@ -51,4 +51,4 @@ manpage: .PHONY: manpages manpages: manpage - go run ./cmd/gen-docs ./manpage + go run ./cmd/gen-docs --man-page --doc-path ./manpage diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index 82efa58f9..9327da20a 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -7,27 +7,53 @@ import ( "github.com/cli/cli/command" "github.com/spf13/cobra/doc" + "github.com/spf13/pflag" ) func main() { - if len(os.Args) < 2 { - fatal("Usage: gen-docs ") - } - dir := os.Args[1] - err := os.MkdirAll(dir, 0755) + var flagError pflag.ErrorHandling + docCmd := pflag.NewFlagSet("", flagError) + var manPage = docCmd.BoolP("man-page", "", false, "Generate manual pages") + var website = docCmd.BoolP("website", "", false, "Generate website pages") + var dir = docCmd.StringP("doc-path", "", "", "Path directory where you want generate doc files") + var help = docCmd.BoolP("help", "h", false, "Help about any command") + + if err := docCmd.Parse(os.Args); err != nil { + os.Exit(1) + } + + if *help { + _, err := fmt.Fprintf(os.Stderr, "Usage of %s:\n\n%s", os.Args[0], docCmd.FlagUsages()) + if err != nil { + fatal(err) + } + os.Exit(1) + } + + if(*dir == "") { + fatal("no dir set") + } + + + err := os.MkdirAll(*dir, 0755) if err != nil { fatal(err) } - err = doc.GenMarkdownTreeCustom(command.RootCmd, dir, filePrepender, linkHandler) - if err != nil { - fatal(err) + if *website { + err = doc.GenMarkdownTreeCustom(command.RootCmd, *dir, filePrepender, linkHandler) + if err != nil { + fatal(err) + } } - err = doc.GenManTree(command.RootCmd, nil, dir) - if err != nil { - fatal(err) + + if *manPage { + err = doc.GenManTree(command.RootCmd, nil, *dir) + if err != nil { + fatal(err) + } } } From 3abc2be0f718c6ff61424c365a006361b37bc892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:17:06 +0200 Subject: [PATCH 06/49] Switch issue create to optimized resolver and update tests --- api/queries_repo_test.go | 130 ++++++++++++++++++++++++++++++++++++++ command/issue.go | 13 ++-- command/issue_test.go | 28 +++----- command/pr_create_test.go | 43 ++++--------- 4 files changed, 156 insertions(+), 58 deletions(-) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 6e36f4abc..94be4001c 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -46,6 +46,136 @@ func Test_RepoCreate(t *testing.T) { t.Errorf("expected homepageUrl to be %q, got %q", "http://example.com", homepage) } } +func Test_RepoMetadata(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + repo := ghrepo.FromFullName("OWNER/REPO") + input := RepoMetadataInput{ + Assignees: true, + Reviewers: true, + Labels: true, + Projects: true, + Milestones: true, + } + + http.Register( + httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\blabels\(`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "feature", "id": "FEATUREID" }, + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\bmilestones\(`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\brepository\(.+\bprojects\(`), + httpmock.StringResponse(` + { "data": { "repository": { "projects": { + "nodes": [ + { "name": "Cleanup", "id": "CLEANUPID" }, + { "name": "Roadmap", "id": "ROADMAPID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\borganization\(.+\bprojects\(`), + httpmock.StringResponse(` + { "data": { "organization": { "projects": { + "nodes": [ + { "name": "Triage", "id": "TRIAGEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`\borganization\(.+\bteams\(`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "owners", "id": "OWNERSID" }, + { "slug": "Core", "id": "COREID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + result, err := RepoMetadata(client, repo, input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedMemberIDs := []string{"MONAID", "HUBOTID"} + memberIDs, err := result.MembersToIDs([]string{"monalisa", "hubot"}) + if err != nil { + t.Errorf("error resolving members: %v", err) + } + if !sliceEqual(memberIDs, expectedMemberIDs) { + t.Errorf("expected members %v, got %v", expectedMemberIDs, memberIDs) + } + + expectedTeamIDs := []string{"COREID", "OWNERSID"} + teamIDs, err := result.TeamsToIDs([]string{"OWNER/core", "/owners"}) + if err != nil { + t.Errorf("error resolving teams: %v", err) + } + if !sliceEqual(teamIDs, expectedTeamIDs) { + t.Errorf("expected teams %v, got %v", expectedTeamIDs, teamIDs) + } + + expectedLabelIDs := []string{"BUGID", "TODOID"} + labelIDs, err := result.LabelsToIDs([]string{"bug", "todo"}) + if err != nil { + t.Errorf("error resolving labels: %v", err) + } + if !sliceEqual(labelIDs, expectedLabelIDs) { + t.Errorf("expected labels %v, got %v", expectedLabelIDs, labelIDs) + } + + expectedProjectIDs := []string{"TRIAGEID", "ROADMAPID"} + projectIDs, err := result.ProjectsToIDs([]string{"triage", "roadmap"}) + if err != nil { + t.Errorf("error resolving projects: %v", err) + } + if !sliceEqual(projectIDs, expectedProjectIDs) { + t.Errorf("expected projects %v, got %v", expectedProjectIDs, projectIDs) + } + + expectedMilestoneID := "BIGONEID" + milestoneID, err := result.MilestoneToID("big one.oh") + if err != nil { + t.Errorf("error resolving milestone: %v", err) + } + if milestoneID != expectedMilestoneID { + t.Errorf("expected milestone %v, got %v", expectedMilestoneID, milestoneID) + } +} func Test_RepoResolveMetadataIDs(t *testing.T) { http := &httpmock.Registry{} diff --git a/command/issue.go b/command/issue.go index b5e44787c..902481c7e 100644 --- a/command/issue.go +++ b/command/issue.go @@ -473,15 +473,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { if tb.HasMetadata() { if tb.MetadataResult == nil { - metadataInput := api.RepoMetadataInput{ - Assignees: len(tb.Assignees) > 0, - Labels: len(tb.Labels) > 0, - Projects: len(tb.Projects) > 0, - Milestones: len(tb.Milestones) > 0, + resolveInput := api.RepoResolveInput{ + Assignees: tb.Assignees, + Labels: tb.Labels, + Projects: tb.Projects, + Milestones: tb.Milestones, } - // TODO: for non-interactive mode, only translate given objects to GraphQL IDs - tb.MetadataResult, err = api.RepoMetadata(apiClient, baseRepo, metadataInput) + tb.MetadataResult, err = api.RepoResolveMetadataIDs(apiClient, baseRepo, resolveInput) if err != nil { return err } diff --git a/command/issue_test.go b/command/issue_test.go index 5a9d4c239..c3a83b75a 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -500,27 +500,15 @@ func TestIssueCreate_metadata(t *testing.T) { } } } `)) http.Register( - httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.GraphQL(`\bu000:`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - http.Register( - httpmock.GraphQL(`\blabels\(`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "feature", "id": "FEATUREID" }, - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "TODO", "id": "TODOID" } + } + } } `)) http.Register( httpmock.GraphQL(`\bmilestones\(`), diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 85966aa72..7f1b772a4 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -87,27 +87,19 @@ func TestPRCreate_metadata(t *testing.T) { ] } } } } `)) http.Register( - httpmock.GraphQL(`\bassignableUsers\(`), + httpmock.GraphQL(`\bteam\(`), httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - http.Register( - httpmock.GraphQL(`\blabels\(`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "feature", "id": "FEATUREID" }, - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } + { "data": { + "u000": { "login": "MonaLisa", "id": "MONAID" }, + "u001": { "login": "hubot", "id": "HUBOTID" }, + "repository": { + "l000": { "name": "bug", "id": "BUGID" }, + "l001": { "name": "TODO", "id": "TODOID" } + }, + "organization": { + "t000": { "slug": "core", "id": "COREID" } + } + } } `)) http.Register( httpmock.GraphQL(`\bmilestones\(`), @@ -139,17 +131,6 @@ func TestPRCreate_metadata(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`\borganization\(.+\bteams\(`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "owners", "id": "OWNERSID" }, - { "slug": "Core", "id": "COREID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`\bcreatePullRequest\(`), httpmock.GraphQLMutation(` From 1cd6e3f9d3b1c460408867c689b74301d232076c Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 12 May 2020 10:13:18 -0400 Subject: [PATCH 07/49] pass some options to GenManTree --- cmd/gen-docs/main.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index 9327da20a..a878e339f 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -50,7 +50,13 @@ func main() { if *manPage { - err = doc.GenManTree(command.RootCmd, nil, *dir) + header := &doc.GenManHeader{ + Title: "gh", + Section: "1", + Source: "", //source and manual are just put at the top of the manpage, before name + Manual: "", //if source is an empty string, it's set to "Auto generated by spf13/cobra" + } + err = doc.GenManTree(command.RootCmd, header, *dir) if err != nil { fatal(err) } From 386a53c34aa2f49bcec23780d16ddafd3ce051c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:48:30 +0200 Subject: [PATCH 08/49] Fix metadata resolver query --- api/queries_repo.go | 2 + api/queries_repo_test.go | 4 +- command/issue.go | 82 +++++++++++++++++++++++++++------------- command/pr_create.go | 45 ++-------------------- 4 files changed, 64 insertions(+), 69 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 6eb7088fc..c8ba31348 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -623,6 +623,7 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes } query := &bytes.Buffer{} + fmt.Fprint(query, "{\n") for i, u := range users { fmt.Fprintf(query, "u%03d: user(login:%q){id,login}\n", i, u) } @@ -640,6 +641,7 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes } fmt.Fprint(query, "}\n") } + fmt.Fprint(query, "}\n") response := make(map[string]json.RawMessage) err = client.GraphQL(query.String(), nil, &response) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 94be4001c..a3a6876a8 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -188,7 +188,8 @@ func Test_RepoResolveMetadataIDs(t *testing.T) { Labels: []string{"bug", "help wanted"}, } - expectedQuery := `u000: user(login:"monalisa"){id,login} + expectedQuery := `{ +u000: user(login:"monalisa"){id,login} u001: user(login:"hubot"){id,login} u002: user(login:"octocat"){id,login} repository(owner:"OWNER",name:"REPO"){ @@ -199,6 +200,7 @@ organization(login:"OWNER"){ t000: team(slug:"core"){id,slug} t001: team(slug:"robots"){id,slug} } +} ` responseJSON := ` { "data": { diff --git a/command/issue.go b/command/issue.go index 902481c7e..19b4072df 100644 --- a/command/issue.go +++ b/command/issue.go @@ -471,25 +471,9 @@ func issueCreate(cmd *cobra.Command, args []string) error { "body": body, } - if tb.HasMetadata() { - if tb.MetadataResult == nil { - resolveInput := api.RepoResolveInput{ - Assignees: tb.Assignees, - Labels: tb.Labels, - Projects: tb.Projects, - Milestones: tb.Milestones, - } - - tb.MetadataResult, err = api.RepoResolveMetadataIDs(apiClient, baseRepo, resolveInput) - if err != nil { - return err - } - } - - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) - if err != nil { - return err - } + err = addMetadataToIssueParams(apiClient, baseRepo, params, &tb) + if err != nil { + return err } newIssue, err := api.IssueCreate(apiClient, repo, params) @@ -505,33 +489,79 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func addMetadataToIssueParams(params map[string]interface{}, metadata *api.RepoMetadataResult, assignees, labelNames, projectNames, milestoneTitles []string) error { - assigneeIDs, err := metadata.MembersToIDs(assignees) +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 := metadata.LabelsToIDs(labelNames) + labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) if err != nil { return fmt.Errorf("could not add label: %w", err) } params["labelIds"] = labelIDs - projectIDs, err := metadata.ProjectsToIDs(projectNames) + 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(milestoneTitles) > 0 { - milestoneID, err := metadata.MilestoneToID(milestoneTitles[0]) + 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", milestoneTitles[0], err) + 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(teamReviewers, 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 } diff --git a/command/pr_create.go b/command/pr_create.go index 9e096c0eb..96dc08791 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -325,48 +325,9 @@ func prCreate(cmd *cobra.Command, _ []string) error { "headRefName": headBranchLabel, } - if tb.HasMetadata() { - if tb.MetadataResult == nil { - resolveInput := api.RepoResolveInput{ - Reviewers: tb.Reviewers, - Assignees: tb.Assignees, - Labels: tb.Labels, - Projects: tb.Projects, - Milestones: tb.Milestones, - } - - tb.MetadataResult, err = api.RepoResolveMetadataIDs(client, baseRepo, resolveInput) - if err != nil { - return err - } - } - - err = addMetadataToIssueParams(params, tb.MetadataResult, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) - if err != nil { - return err - } - - var userReviewers []string - var teamReviewers []string - for _, r := range tb.Reviewers { - if strings.ContainsRune(r, '/') { - teamReviewers = append(teamReviewers, r) - } else { - userReviewers = append(teamReviewers, 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 + err = addMetadataToIssueParams(client, baseRepo, params, &tb) + if err != nil { + return err } pr, err := api.CreatePullRequest(client, baseRepo, params) From b75c4a812d89d65312567deca2046d8b32feaba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 16:49:04 +0200 Subject: [PATCH 09/49] Guard against leaked parameters in issue/pr create tests --- command/issue_test.go | 6 ++++++ command/pr_create_test.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/command/issue_test.go b/command/issue_test.go index c3a83b75a..3df6910f5 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -556,6 +556,12 @@ func TestIssueCreate_metadata(t *testing.T) { eq(t, inputs["labelIds"], []interface{}{"BUGID", "TODOID"}) eq(t, inputs["projectIds"], []interface{}{"ROADMAPID"}) eq(t, inputs["milestoneId"], "BIGONEID") + if v, ok := inputs["userIds"]; ok { + t.Errorf("did not expect userIds: %v", v) + } + if v, ok := inputs["teamIds"]; ok { + t.Errorf("did not expect teamIds: %v", v) + } })) output, err := RunCommand(`issue create -t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh'`) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 7f1b772a4..18a54e887 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -141,6 +141,12 @@ func TestPRCreate_metadata(t *testing.T) { `, func(inputs map[string]interface{}) { eq(t, inputs["title"], "TITLE") eq(t, inputs["body"], "BODY") + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + if v, ok := inputs["userIds"]; ok { + t.Errorf("did not expect userIds: %v", v) + } })) http.Register( httpmock.GraphQL(`\bupdatePullRequest\(`), From c225d379a9599041b784c2be31e00380f66502a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 13 May 2020 12:09:59 +0200 Subject: [PATCH 10/49] Preserve CODEOWNERS reviewers in `pr create` When reviewers were requested on a PR, they would apparently overwrite the current set of reviewers. A fresh PR will already have reviewers if it was assigned some by CODEOWNERS rules. The fix is to only ever add additional reviewers and not overwrite the entire set. --- api/queries_pr.go | 1 + command/pr_create_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index c1afc6bc9..c0b16f3c4 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -667,6 +667,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter requestReviews(input: $input) { clientMutationId } }` reviewParams["pullRequestId"] = pr.ID + reviewParams["union"] = true variables := map[string]interface{}{ "input": reviewParams, } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 85966aa72..d6b89a6b0 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -184,6 +184,7 @@ func TestPRCreate_metadata(t *testing.T) { eq(t, inputs["pullRequestId"], "NEWPULLID") eq(t, inputs["userIds"], []interface{}{"HUBOTID"}) eq(t, inputs["teamIds"], []interface{}{"COREID"}) + eq(t, inputs["union"], true) })) cs, cmdTeardown := test.InitCmdStubber() From 1ff37be361fb7253b8ce6a3f1aaf744b8c6cbfb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 13 May 2020 17:16:12 +0200 Subject: [PATCH 11/49] Fix assigning multiple user reviewers This was due to a typo. Fixes #913 --- command/issue.go | 2 +- command/pr_create_test.go | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/command/issue.go b/command/issue.go index 19b4072df..5eed13a13 100644 --- a/command/issue.go +++ b/command/issue.go @@ -546,7 +546,7 @@ func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par if strings.ContainsRune(r, '/') { teamReviewers = append(teamReviewers, r) } else { - userReviewers = append(teamReviewers, r) + userReviewers = append(userReviewers, r) } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 18a54e887..f0cc34363 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -97,7 +97,8 @@ func TestPRCreate_metadata(t *testing.T) { "l001": { "name": "TODO", "id": "TODOID" } }, "organization": { - "t000": { "slug": "core", "id": "COREID" } + "t000": { "slug": "core", "id": "COREID" }, + "t001": { "slug": "robots", "id": "ROBOTID" } } } } `)) @@ -169,8 +170,8 @@ func TestPRCreate_metadata(t *testing.T) { } } } `, func(inputs map[string]interface{}) { eq(t, inputs["pullRequestId"], "NEWPULLID") - eq(t, inputs["userIds"], []interface{}{"HUBOTID"}) - eq(t, inputs["teamIds"], []interface{}{"COREID"}) + eq(t, inputs["userIds"], []interface{}{"HUBOTID", "MONAID"}) + eq(t, inputs["teamIds"], []interface{}{"COREID", "ROBOTID"}) })) cs, cmdTeardown := test.InitCmdStubber() @@ -182,7 +183,7 @@ 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 /core`) + 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`) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") From e3676c3a95085c25c74d97cf851dcc965b979b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 May 2020 18:32:57 +0200 Subject: [PATCH 12/49] Change source installation docs to avoid suggesting to change PATH --- docs/source.md | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/docs/source.md b/docs/source.md index 7df8bfe60..059ef2094 100644 --- a/docs/source.md +++ b/docs/source.md @@ -1,37 +1,29 @@ # Installation from source 0. Verify that you have Go 1.14+ installed -``` -$ go version -go version go1.14 -``` -1. Clone cli into `~/.githubcli` -``` -$ git clone https://github.com/cli/cli.git ~/.githubcli -``` + ```sh + $ go version + go version go1.14 + ``` -2. Compile -``` -$ cd ~/.githubcli && make -``` +1. Clone this repository -3. Add `~/.githubcli/bin` to your $PATH for access to the gh command-line utility. + ```sh + $ git clone https://github.com/cli/cli.git gh-cli + $ cd gh-cli + ``` - * For **bash**: - ~~~ bash - $ echo 'export PATH="$HOME/.githubcli/bin:$PATH"' >> ~/.bash_profile - ~~~ - - * For **Zsh**: - ~~~ zsh - $ echo 'export PATH="$HOME/.githubcli/bin:$PATH"' >> ~/.zshrc - ~~~ - - * For **Fish shell**: - ~~~ fish - $ set -Ux fish_user_paths $HOME/.githubcli/bin $fish_user_paths - ~~~ +2. Build the project -4. Restart your shell so that PATH changes take effect. + ``` + $ make + ``` +3. Move the resulting `bin/gh` executable to somewhere in your PATH + + ```sh + $ sudo mv ./bin/gh /usr/local/bin/ + ``` + +4. Run `gh version` to check if it worked. From c8c807b954c40811b6b7391a5cd7ea661269c510 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 13 May 2020 14:55:49 -0500 Subject: [PATCH 13/49] pass apiClient to determineBaseRepo Our code had an unspoken assumption that only one apiClient is created during the course of a command. Violating this assumption is fine in almost all cases, but not when we need to do a re-auth to add a new oauth scope to a user's token. There is likely a more elegant solution to the problem but until then this changes determineBaseRepo to use an existing apiClient. --- command/issue.go | 21 ++++++++++----------- command/pr.go | 12 ++++++------ command/pr_checkout.go | 2 +- command/pr_review.go | 10 +++++----- command/repo.go | 12 ++++++------ command/root.go | 7 +------ 6 files changed, 29 insertions(+), 35 deletions(-) diff --git a/command/issue.go b/command/issue.go index 5eed13a13..3c7727e53 100644 --- a/command/issue.go +++ b/command/issue.go @@ -106,7 +106,7 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -167,7 +167,7 @@ func issueStatus(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -224,7 +224,7 @@ func issueView(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -340,9 +340,13 @@ func issueFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) func issueCreate(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } // NB no auto forking like over in pr create - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -406,11 +410,6 @@ func issueCreate(cmd *cobra.Command, args []string) error { fmt.Fprintf(colorableErr(cmd), "\nCreating issue in %s\n\n", ghrepo.FullName(baseRepo)) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - repo, err := api.GitHubRepo(apiClient, baseRepo) if err != nil { return err @@ -654,7 +653,7 @@ func issueClose(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -689,7 +688,7 @@ func issueReopen(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } diff --git a/command/pr.go b/command/pr.go index 2407e0867..ee529aa9f 100644 --- a/command/pr.go +++ b/command/pr.go @@ -104,7 +104,7 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -168,7 +168,7 @@ func prList(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -307,7 +307,7 @@ func prView(cmd *cobra.Command, args []string) error { } if baseRepo == nil { - baseRepo, err = determineBaseRepo(cmd, ctx) + baseRepo, err = determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -366,7 +366,7 @@ func prClose(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -401,7 +401,7 @@ func prReopen(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -438,7 +438,7 @@ func prMerge(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 7da76f6ce..aa9e3afcc 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -34,7 +34,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { } if baseRepo == nil { - baseRepo, err = determineBaseRepo(cmd, ctx) + baseRepo, err = determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } diff --git a/command/pr_review.go b/command/pr_review.go index d51aaff03..3b1a4ab88 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -86,16 +86,16 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { func prReview(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) - baseRepo, err := determineBaseRepo(cmd, ctx) - if err != nil { - return fmt.Errorf("could not determine base repo: %w", err) - } - apiClient, err := apiClientForContext(ctx) if err != nil { return err } + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + if err != nil { + return fmt.Errorf("could not determine base repo: %w", err) + } + var prNum int branchWithOwner := "" diff --git a/command/repo.go b/command/repo.go index 982a98595..e5cbe99bc 100644 --- a/command/repo.go +++ b/command/repo.go @@ -336,7 +336,7 @@ func repoFork(cmd *cobra.Command, args []string) error { var repoToFork ghrepo.Interface inParent := false // whether or not we're forking the repo we're currently "in" if len(args) == 0 { - baseRepo, err := determineBaseRepo(cmd, ctx) + baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) if err != nil { return fmt.Errorf("unable to determine base repository: %w", err) } @@ -487,11 +487,15 @@ var Confirm = func(prompt string, result *bool) error { func repoView(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } var toView ghrepo.Interface if len(args) == 0 { var err error - toView, err = determineBaseRepo(cmd, ctx) + toView, err = determineBaseRepo(apiClient, cmd, ctx) if err != nil { return err } @@ -512,10 +516,6 @@ func repoView(cmd *cobra.Command, args []string) error { } } - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } repo, err := api.GitHubRepo(apiClient, toView) if err != nil { return err diff --git a/command/root.go b/command/root.go index ef88a3184..df2883d40 100644 --- a/command/root.go +++ b/command/root.go @@ -216,17 +216,12 @@ func changelogURL(version string) string { return url } -func determineBaseRepo(cmd *cobra.Command, ctx context.Context) (ghrepo.Interface, error) { +func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Context) (ghrepo.Interface, error) { repo, err := cmd.Flags().GetString("repo") if err == nil && repo != "" { return ghrepo.FromFullName(repo), nil } - apiClient, err := apiClientForContext(ctx) - if err != nil { - return nil, err - } - baseOverride, err := cmd.Flags().GetString("repo") if err != nil { return nil, err From d440a95aed76145e8a90d01a3e5948ba5d0cec08 Mon Sep 17 00:00:00 2001 From: Kevin Bluer Date: Mon, 18 May 2020 04:13:48 -0500 Subject: [PATCH 14/49] Improved error message when "owner/repo" format not provided (#919) Fixes #882 --- api/queries_issue_test.go | 3 ++- api/queries_repo_test.go | 4 ++-- command/repo.go | 17 ++++++++++++----- command/root.go | 6 +++++- context/blank_context.go | 3 ++- context/context.go | 4 ++-- internal/ghrepo/repo.go | 11 ++++++----- 7 files changed, 31 insertions(+), 17 deletions(-) diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index c8ee505cf..ffe4aaad1 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -39,7 +39,8 @@ func TestIssueList(t *testing.T) { } } } `)) - _, err := IssueList(client, ghrepo.FromFullName("OWNER/REPO"), "open", []string{}, "", 251, "") + repo, _ := ghrepo.FromFullName("OWNER/REPO") + _, err := IssueList(client, repo, "open", []string{}, "", 251, "") if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index a3a6876a8..03123cf43 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -50,7 +50,7 @@ func Test_RepoMetadata(t *testing.T) { http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) - repo := ghrepo.FromFullName("OWNER/REPO") + repo, _ := ghrepo.FromFullName("OWNER/REPO") input := RepoMetadataInput{ Assignees: true, Reviewers: true, @@ -181,7 +181,7 @@ func Test_RepoResolveMetadataIDs(t *testing.T) { http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) - repo := ghrepo.FromFullName("OWNER/REPO") + repo, _ := ghrepo.FromFullName("OWNER/REPO") input := RepoResolveInput{ Assignees: []string{"monalisa", "hubot"}, Reviewers: []string{"monalisa", "octocat", "OWNER/core", "/robots"}, diff --git a/command/repo.go b/command/repo.go index 982a98595..67e9393ea 100644 --- a/command/repo.go +++ b/command/repo.go @@ -189,7 +189,10 @@ func repoCreate(cmd *cobra.Command, args []string) error { if len(args) > 0 { name = args[0] if strings.Contains(name, "/") { - newRepo := ghrepo.FromFullName(name) + newRepo, err := ghrepo.FromFullName(name) + if err != nil { + return fmt.Errorf("argument error: %w", err) + } orgName = newRepo.RepoOwner() name = newRepo.RepoName() } @@ -366,9 +369,9 @@ func repoFork(cmd *cobra.Command, args []string) error { return fmt.Errorf("did not understand argument: %w", err) } } else { - repoToFork = ghrepo.FromFullName(repoArg) - if repoToFork.RepoName() == "" || repoToFork.RepoOwner() == "" { - return fmt.Errorf("could not parse owner or repo name from %s", repoArg) + repoToFork, err = ghrepo.FromFullName(repoArg) + if err != nil { + return fmt.Errorf("argument error: %w", err) } } } @@ -508,7 +511,11 @@ func repoView(cmd *cobra.Command, args []string) error { return fmt.Errorf("did not understand argument: %w", err) } } else { - toView = ghrepo.FromFullName(repoArg) + var err error + toView, err = ghrepo.FromFullName(repoArg) + if err != nil { + return fmt.Errorf("argument error: %w", err) + } } } diff --git a/command/root.go b/command/root.go index ef88a3184..8b807e622 100644 --- a/command/root.go +++ b/command/root.go @@ -219,7 +219,11 @@ func changelogURL(version string) string { func determineBaseRepo(cmd *cobra.Command, ctx context.Context) (ghrepo.Interface, error) { repo, err := cmd.Flags().GetString("repo") if err == nil && repo != "" { - return ghrepo.FromFullName(repo), nil + baseRepo, err := ghrepo.FromFullName(repo) + if err != nil { + return nil, fmt.Errorf("argument error: %w", err) + } + return baseRepo, nil } apiClient, err := apiClientForContext(ctx) diff --git a/context/blank_context.go b/context/blank_context.go index c5ceddff7..343f32c7e 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -92,5 +92,6 @@ func (c *blankContext) BaseRepo() (ghrepo.Interface, error) { } func (c *blankContext) SetBaseRepo(nwo string) { - c.baseRepo = ghrepo.FromFullName(nwo) + repo, _ := ghrepo.FromFullName(nwo) + c.baseRepo = repo } diff --git a/context/context.go b/context/context.go index a5cd823fc..2a25f5dda 100644 --- a/context/context.go +++ b/context/context.go @@ -39,7 +39,7 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re } hasBaseOverride := base != "" - baseOverride := ghrepo.FromFullName(base) + baseOverride, _ := ghrepo.FromFullName(base) foundBaseOverride := false repos := make([]ghrepo.Interface, 0, lenRemotesForLookup) for _, r := range remotes[:lenRemotesForLookup] { @@ -266,5 +266,5 @@ func (c *fsContext) BaseRepo() (ghrepo.Interface, error) { } func (c *fsContext) SetBaseRepo(nwo string) { - c.baseRepo = ghrepo.FromFullName(nwo) + c.baseRepo, _ = ghrepo.FromFullName(nwo) } diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index dc3115b90..37fecbe8d 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -28,14 +28,15 @@ func FullName(r Interface) string { return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName()) } -// FromFullName extracts the GitHub repository inforation from an "OWNER/REPO" string -func FromFullName(nwo string) Interface { +// FromFullName extracts the GitHub repository information from an "OWNER/REPO" string +func FromFullName(nwo string) (Interface, error) { var r ghRepo parts := strings.SplitN(nwo, "/", 2) - if len(parts) == 2 { - r.owner, r.name = parts[0], parts[1] + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return &r, fmt.Errorf("expected OWNER/REPO format, got %q", nwo) } - return &r + r.owner, r.name = parts[0], parts[1] + return &r, nil } // FromURL extracts the GitHub repository information from a URL From 228d01a037eef834f0c634cb694f7385214da6bf Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 10:23:20 -0500 Subject: [PATCH 15/49] reenable skipping body prompt for pr/issue --- command/title_body_survey.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 17f7af9fd..0fb39d349 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -159,6 +159,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie bodyQuestion := &survey.Question{ Name: "body", Prompt: &surveyext.GhEditor{ + BlankAllowed: true, EditorCommand: editorCommand, Editor: &survey.Editor{ Message: "Body", From 350b4c85c08d98b02a65953304c3a88ee12d0201 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 11:11:02 -0700 Subject: [PATCH 16/49] Add failing test --- command/pr_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index 528e2ee77..153cd7a3c 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1093,3 +1093,26 @@ func TestPrMerge_alreadyMerged(t *testing.T) { 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": 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 ready for review`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From fad9f24f39f7851855f30f34ea0af37c10c990e6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 11:11:13 -0700 Subject: [PATCH 17/49] Make reopen work --- api/queries_pr.go | 22 ++++++++++++++++++++++ command/pr.go | 43 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index c0b16f3c4..6e5306f78 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -953,6 +953,28 @@ func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest, m return err } +func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) error { + var mutation struct { + MarkPullRequestReadyForReviewInput struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + } + + input := struct { + // ID of the pull request to be reopened. (Required.) + PullRequestID githubv4.ID `json:"pullRequestId"` + }{ + PullRequestID: pr.ID, + } + + v4 := githubv4.NewClient(client.http) + err := v4.Mutate(context.Background(), &mutation, input, nil) + + return err +} + func min(a, b int) int { if a < b { return a diff --git a/command/pr.go b/command/pr.go index ee529aa9f..fdf957c3b 100644 --- a/command/pr.go +++ b/command/pr.go @@ -29,6 +29,7 @@ func init() { prMergeCmd.Flags().BoolP("merge", "m", true, "Merge the commits with the base branch") prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") + prCmd.AddCommand(prReadyCmd) prCmd.AddCommand(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") @@ -84,13 +85,18 @@ var prReopenCmd = &cobra.Command{ Args: cobra.ExactArgs(1), RunE: prReopen, } - var prMergeCmd = &cobra.Command{ Use: "merge [ | | ]", Short: "Merge a pull request", Args: cobra.MaximumNArgs(1), RunE: prMerge, } +var prReadyCmd = &cobra.Command{ + Use: "ready [ | | ]", + Short: "Make a pull request as ready for review", + Args: cobra.MaximumNArgs(1), + RunE: prReady, +} func prStatus(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) @@ -551,6 +557,41 @@ func printPrPreview(out io.Writer, pr *api.PullRequest) error { return nil } +func prReady(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 + } + + pr, err := prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + + // if pr.State == "MERGED" { + // err := fmt.Errorf("%s Pull request #%d can't be closed because it was already merged", utils.Red("!"), pr.Number) + // return err + // } else if pr.Closed { + // fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\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 ready for review\n", utils.Green("✔"), pr.Number) + + return nil +} + // Ref. https://developer.github.com/v4/enum/pullrequestreviewstate/ const ( requestedReviewState = "REQUESTED" // This is our own state for review request From 296062b8691a111420c270cc2e7d47c2172d120d Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 14:00:19 -0500 Subject: [PATCH 18/49] configure author on site commits --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index f2b4805c8..beea7c099 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,7 @@ site-docs: site for f in site/manual/gh*.md; do sed -i.bak -e '/^### SEE ALSO/,$$d' "$$f"; done rm -f site/manual/*.bak git -C site add 'manual/gh*.md' - git -C site commit -m 'update help docs' || true + git -C site commit --author "cli automation " -m 'update help docs' || true .PHONY: site-docs site-publish: site-docs @@ -41,6 +41,6 @@ ifndef GITHUB_REF endif sed -i.bak -E 's/(assign version = )".+"/\1"$(GITHUB_REF:refs/tags/v%=%)"/' site/index.html rm -f site/index.html.bak - git -C site commit -m '$(GITHUB_REF:refs/tags/v%=%)' index.html + git -C site commit --author "cli automation " -m '$(GITHUB_REF:refs/tags/v%=%)' index.html git -C site push .PHONY: site-publish From da95cbd1bf81efd9ee5fd78153ef2d0108ba026f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 14:05:50 -0500 Subject: [PATCH 19/49] use gh if available when cloning site --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index beea7c099..3fad3142e 100644 --- a/Makefile +++ b/Makefile @@ -23,7 +23,7 @@ test: .PHONY: test site: - git clone https://github.com/github/cli.github.com.git "$@" + (which gh 2>/dev/null && gh repo clone github/cli.github.com "$@") || git clone https://github.com/github/cli.github.com.git "$@" site-docs: site git -C site pull From 922b6e06e0482b89287d9c509ce89bcf1afb486a Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 12:42:16 -0700 Subject: [PATCH 20/49] Can be marked ready for review --- api/queries_pr.go | 7 +++---- command/pr.go | 23 ++++++++++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 6e5306f78..b3d380240 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -962,13 +962,12 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er } `graphql:"markPullRequestReadyForReview(input: $input)"` } - input := struct { - // ID of the pull request to be reopened. (Required.) + type MarkPullRequestReadyForReviewInput struct { PullRequestID githubv4.ID `json:"pullRequestId"` - }{ - PullRequestID: pr.ID, } + input := MarkPullRequestReadyForReviewInput{PullRequestID: pr.ID} + v4 := githubv4.NewClient(client.http) err := v4.Mutate(context.Background(), &mutation, input, nil) diff --git a/command/pr.go b/command/pr.go index fdf957c3b..26b2bc76d 100644 --- a/command/pr.go +++ b/command/pr.go @@ -569,9 +569,26 @@ func prReady(cmd *cobra.Command, args []string) error { return err } - pr, err := prFromArg(apiClient, baseRepo, args[0]) - if err != nil { - return err + var pr *api.PullRequest + if len(args) > 0 { + pr, err = prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + } else { + prNumber, branchWithOwner, err := prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil { + return err + } + + if prNumber != 0 { + pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNumber) + } else { + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + } + if err != nil { + return err + } } // if pr.State == "MERGED" { From 0b0070e725c52db9394604e826bf88234c4ce52b Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 13:03:43 -0700 Subject: [PATCH 21/49] Add "already marked" test --- command/pr.go | 16 ++++++++-------- command/pr_test.go | 27 +++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/command/pr.go b/command/pr.go index 26b2bc76d..87658da42 100644 --- a/command/pr.go +++ b/command/pr.go @@ -591,20 +591,20 @@ func prReady(cmd *cobra.Command, args []string) error { } } - // if pr.State == "MERGED" { - // err := fmt.Errorf("%s Pull request #%d can't be closed because it was already merged", utils.Red("!"), pr.Number) - // return err - // } else if pr.Closed { - // fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) - // return nil - // } + if !pr.IsDraft { + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d was already marked as \"ready for review\"\n", utils.Yellow("!"), pr.Number) + return nil + } else if pr.Closed { + err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"\n", utils.Red("!"), pr.Number) + return err + } 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 ready for review\n", utils.Green("✔"), pr.Number) + 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_test.go b/command/pr_test.go index 153cd7a3c..2a79ed4cf 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1100,7 +1100,7 @@ func TestPRReady(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { - "pullRequest": { "number": 444, "closed": true} + "pullRequest": { "number": 444, "closed": false, "isDraft": true} } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) @@ -1110,7 +1110,30 @@ func TestPRReady(t *testing.T) { t.Fatalf("error running command `pr ready`: %v", err) } - r := regexp.MustCompile(`Pull request #444 is marked ready for review`) + 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": 444, "closed": false, "isDraft": false} + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand("pr ready 444") + if err != nil { + t.Fatalf("expected an error running command `pr ready` on a review that is already ready!: %v", err) + } + + r := regexp.MustCompile(`Pull request #444 was already 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()) From d800e3f0b695ef06e1ca5be3051c9759a2973b54 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 15:07:28 -0500 Subject: [PATCH 22/49] rely on environment --- .github/workflows/releases.yml | 5 +++++ Makefile | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 73974448c..b7fc01545 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -44,6 +44,11 @@ jobs: token: ${{secrets.SITE_GITHUB_TOKEN}} - name: Publish documentation site if: "!contains(github.ref, '-')" # skip prereleases + env: + GIT_COMMITTER_NAME: cli automation + GIT_AUTHOR_NAME: cli automation + GIT_COMMITTER_EMAIL: noreply@github.com + GIT_AUTHOR_EMAIL: noreply@github.com run: make site-publish - name: Move project cards if: "!contains(github.ref, '-')" # skip prereleases diff --git a/Makefile b/Makefile index 3fad3142e..4fcb161bf 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,7 @@ site-docs: site for f in site/manual/gh*.md; do sed -i.bak -e '/^### SEE ALSO/,$$d' "$$f"; done rm -f site/manual/*.bak git -C site add 'manual/gh*.md' - git -C site commit --author "cli automation " -m 'update help docs' || true + git -C site commit -m 'update help docs' || true .PHONY: site-docs site-publish: site-docs @@ -41,6 +41,6 @@ ifndef GITHUB_REF endif sed -i.bak -E 's/(assign version = )".+"/\1"$(GITHUB_REF:refs/tags/v%=%)"/' site/index.html rm -f site/index.html.bak - git -C site commit --author "cli automation " -m '$(GITHUB_REF:refs/tags/v%=%)' index.html + git -C site commit -m '$(GITHUB_REF:refs/tags/v%=%)' index.html git -C site push .PHONY: site-publish From d2bdd53dd27af163600e661edd8b7287632fb218 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 15:08:31 -0500 Subject: [PATCH 23/49] depend on bin/gh --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4fcb161bf..ecc5d819f 100644 --- a/Makefile +++ b/Makefile @@ -22,8 +22,8 @@ test: go test ./... .PHONY: test -site: - (which gh 2>/dev/null && gh repo clone github/cli.github.com "$@") || git clone https://github.com/github/cli.github.com.git "$@" +site: bin/gh + gh repo clone github/cli.github.com "$@" site-docs: site git -C site pull From 78bc7260e0f73d6de2cca54b0327d3de61a68c52 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 13:08:57 -0700 Subject: [PATCH 24/49] Add closed test --- command/pr.go | 9 +++++---- command/pr_test.go | 31 +++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/command/pr.go b/command/pr.go index 87658da42..7df7522b7 100644 --- a/command/pr.go +++ b/command/pr.go @@ -591,12 +591,13 @@ func prReady(cmd *cobra.Command, args []string) error { } } - if !pr.IsDraft { + if pr.Closed { + err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"\n", utils.Red("!"), pr.Number) + fmt.Printf("🌭 %+v\n", "huh") + return err + } else if !pr.IsDraft { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d was already marked as \"ready for review\"\n", utils.Yellow("!"), pr.Number) return nil - } else if pr.Closed { - err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"\n", utils.Red("!"), pr.Number) - return err } err = api.PullRequestReady(apiClient, baseRepo, pr) diff --git a/command/pr_test.go b/command/pr_test.go index 2a79ed4cf..64b1151fb 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1123,19 +1123,42 @@ func TestPRReady_alreadyReady(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { - "pullRequest": { "number": 444, "closed": false, "isDraft": false} + "pullRequest": { "number": 445, "closed": false, "isDraft": false} } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - output, err := RunCommand("pr ready 444") + output, err := RunCommand("pr ready 445") if err != nil { - t.Fatalf("expected an error running command `pr ready` on a review that is already ready!: %v", err) + t.Fatalf("error running command `pr ready`: %v", err) } - r := regexp.MustCompile(`Pull request #444 was already marked as "ready for review"`) + r := regexp.MustCompile(`Pull request #445 was already 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_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 and can't be marked "ready for review"`) + + if !r.MatchString(err.Error()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, err.Error()) + } +} From 14e8498b9072dd8766e7d9a0c443c048dd5e70a4 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 15:14:10 -0500 Subject: [PATCH 25/49] fix path --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index ecc5d819f..3d7bd0f96 100644 --- a/Makefile +++ b/Makefile @@ -23,7 +23,7 @@ test: .PHONY: test site: bin/gh - gh repo clone github/cli.github.com "$@" + bin/gh repo clone github/cli.github.com "$@" site-docs: site git -C site pull From b42f5527fd81639d85bf2691c05fb2fb8322fd1e Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 13:27:23 -0700 Subject: [PATCH 26/49] Remove hot dog --- command/pr.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 7df7522b7..863e7c043 100644 --- a/command/pr.go +++ b/command/pr.go @@ -592,8 +592,7 @@ func prReady(cmd *cobra.Command, args []string) error { } if pr.Closed { - err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"\n", utils.Red("!"), pr.Number) - fmt.Printf("🌭 %+v\n", "huh") + err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"", utils.Red("!"), pr.Number) return err } else if !pr.IsDraft { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d was already marked as \"ready for review\"\n", utils.Yellow("!"), pr.Number) From b94860bae7b25f384de7a084d314c470e8616279 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 15:43:27 -0500 Subject: [PATCH 27/49] add command file with pr resolution --- command/pr_diff.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 command/pr_diff.go diff --git a/command/pr_diff.go b/command/pr_diff.go new file mode 100644 index 000000000..db44bdef3 --- /dev/null +++ b/command/pr_diff.go @@ -0,0 +1,75 @@ +package command + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/spf13/cobra" +) + +var prDiffCmd = &cobra.Command{ + Use: "diff { | }", + Short: "View a pull request's changes.", + RunE: prDiff, +} + +func init() { + prDiffCmd.Flags().StringP("color", "c", "auto", "Whether or not to output color: {always|never|auto}") + + prCmd.AddCommand(prDiffCmd) +} + +func prDiff(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 fmt.Errorf("could not determine base repo: %w", err) + } + + var prNum int + branchWithOwner := "" + + if len(args) == 0 { + prNum, branchWithOwner, err = prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil { + return fmt.Errorf("could not query for pull request for current branch: %w", err) + } + } else { + prArg, repo := prFromURL(args[0]) + if repo != nil { + baseRepo = repo + } else { + prArg = strings.TrimPrefix(args[0], "#") + } + prNum, err = strconv.Atoi(prArg) + if err != nil { + return errors.New("could not parse pull request argument") + } + } + + var pr *api.PullRequest + if prNum > 0 { + pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNum) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + } else { + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + prNum = pr.Number + } + + fmt.Println(pr.Title) + + return nil +} From 9721f75b2b1370ada29b81d8d69e67394d90fdb1 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 14:39:29 -0700 Subject: [PATCH 28/49] Update command/pr.go Co-authored-by: Billy Griffin <5091167+billygriffin@users.noreply.github.com> --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 863e7c043..de85878d6 100644 --- a/command/pr.go +++ b/command/pr.go @@ -592,7 +592,7 @@ func prReady(cmd *cobra.Command, args []string) error { } if pr.Closed { - err := fmt.Errorf("%s Pull request #%d is closed and can't be marked \"ready for review\"", utils.Red("!"), pr.Number) + 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 was already marked as \"ready for review\"\n", utils.Yellow("!"), pr.Number) From 6eab3751d05528bd94c9679d10baed495ae468c7 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 14:39:43 -0700 Subject: [PATCH 29/49] Update command/pr.go Co-authored-by: Billy Griffin <5091167+billygriffin@users.noreply.github.com> --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index de85878d6..cf3eaa8f7 100644 --- a/command/pr.go +++ b/command/pr.go @@ -595,7 +595,7 @@ func prReady(cmd *cobra.Command, args []string) error { 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 was already marked as \"ready for review\"\n", utils.Yellow("!"), pr.Number) + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already \"ready for review\"\n", utils.Yellow("!"), pr.Number) return nil } From be927b34ae186488663c8c2f3aa7553daaef3270 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 14:39:52 -0700 Subject: [PATCH 30/49] Update command/pr_test.go Co-authored-by: Billy Griffin <5091167+billygriffin@users.noreply.github.com> --- command/pr_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index 64b1151fb..ebbee4aff 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1133,7 +1133,7 @@ func TestPRReady_alreadyReady(t *testing.T) { t.Fatalf("error running command `pr ready`: %v", err) } - r := regexp.MustCompile(`Pull request #445 was already marked as "ready for review"`) + 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()) From fde5409a701d24acf8b0df3e32c670fc5024f08e Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 18 May 2020 14:40:04 -0700 Subject: [PATCH 31/49] Update command/pr_test.go Co-authored-by: Billy Griffin <5091167+billygriffin@users.noreply.github.com> --- command/pr_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index ebbee4aff..24fce9d63 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -1156,7 +1156,7 @@ func TestPRReady_closed(t *testing.T) { 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 and can't be marked "ready for review"`) + 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()) From aaebdfc46f1da12868560fed327333923a80b6bc Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 16:49:51 -0500 Subject: [PATCH 32/49] working with gross colorize hack + no pager --- api/queries_pr.go | 26 ++++++++++++++++++++++++++ command/pr_diff.go | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index c0b16f3c4..80428ae07 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -3,6 +3,8 @@ package api import ( "context" "fmt" + "io/ioutil" + "net/http" "strings" "github.com/shurcooL/githubv4" @@ -201,6 +203,30 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { return } +func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNum int) (string, error) { + url := fmt.Sprintf("https://api.github.com/repos/%s/pulls/%d", + ghrepo.FullName(baseRepo), prNum) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return "", err + } + + req.Header.Set("Accept", "application/vnd.github.v3.diff; charset=utf-8") + + resp, err := c.http.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return "", err + } + + return string(b), nil +} + func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { type edges struct { TotalCount int diff --git a/command/pr_diff.go b/command/pr_diff.go index db44bdef3..56e52f551 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -3,10 +3,12 @@ package command import ( "errors" "fmt" + "os" "strconv" "strings" "github.com/cli/cli/api" + "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -34,6 +36,7 @@ func prDiff(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not determine base repo: %w", err) } + // begin pr resolution boilerplate var prNum int branchWithOwner := "" @@ -68,8 +71,44 @@ func prDiff(cmd *cobra.Command, args []string) error { } prNum = pr.Number } + // end pr resolution boilerplate - fmt.Println(pr.Title) + diff, err := apiClient.PullRequestDiff(baseRepo, prNum) + if err != nil { + return err + } + + color, err := cmd.Flags().GetString("color") + if err != nil { + return err + } + + out := cmd.OutOrStdout() + if color == "auto" { + color = "never" + isTTY := false + if outFile, isFile := out.(*os.File); isFile { + isTTY = utils.IsTerminal(outFile) + if isTTY { + color = "always" + } + } + } + + switch color { + case "always": + out = colorableOut(cmd) + rendered, err := utils.RenderMarkdown(fmt.Sprintf("```diff\n%s\n```", diff)) + fmt.Fprintf(out, rendered) + if err != nil { + return fmt.Errorf("failed to colorize diff: %w", err) + } + case "never": + out := cmd.OutOrStdout() + fmt.Fprintf(out, diff) + default: + return fmt.Errorf("did not understand color setting %q", color) + } return nil } From c159d41fc205e2295b018fdebe97fdf5e098b03e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:15:48 -0500 Subject: [PATCH 33/49] manually colorize --- command/pr_diff.go | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 56e52f551..90cc608f3 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -98,10 +98,18 @@ func prDiff(cmd *cobra.Command, args []string) error { switch color { case "always": out = colorableOut(cmd) - rendered, err := utils.RenderMarkdown(fmt.Sprintf("```diff\n%s\n```", diff)) - fmt.Fprintf(out, rendered) - if err != nil { - return fmt.Errorf("failed to colorize diff: %w", err) + for _, diffLine := range strings.Split(diff, "\n") { + output := diffLine + switch { + case bold(diffLine): + output = utils.Bold(diffLine) + case green(diffLine): + output = utils.Green(diffLine) + case red(diffLine): + output = utils.Red(diffLine) + } + + fmt.Fprintln(out, output) } case "never": out := cmd.OutOrStdout() @@ -112,3 +120,21 @@ func prDiff(cmd *cobra.Command, args []string) error { return nil } + +func bold(dl string) bool { + prefixes := []string{"+++", "---", "diff", "index"} + for _, p := range prefixes { + if strings.HasPrefix(dl, p) { + return true + } + } + return false +} + +func red(dl string) bool { + return strings.HasPrefix(dl, "-") +} + +func green(dl string) bool { + return strings.HasPrefix(dl, "+") +} From 8e6b8d39014cd691f96ff76b7640735283a8e25f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:23:36 -0500 Subject: [PATCH 34/49] cleanup --- command/pr_diff.go | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 90cc608f3..622b50edd 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -82,6 +82,9 @@ func prDiff(cmd *cobra.Command, args []string) error { if err != nil { return err } + if !validColor(color) { + return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) + } out := cmd.OutOrStdout() if color == "auto" { @@ -95,27 +98,24 @@ func prDiff(cmd *cobra.Command, args []string) error { } } - switch color { - case "always": - out = colorableOut(cmd) - for _, diffLine := range strings.Split(diff, "\n") { - output := diffLine - switch { - case bold(diffLine): - output = utils.Bold(diffLine) - case green(diffLine): - output = utils.Green(diffLine) - case red(diffLine): - output = utils.Red(diffLine) - } + if color == "never" { + fmt.Fprint(out, diff) + return nil + } - fmt.Fprintln(out, output) + out = colorableOut(cmd) + for _, diffLine := range strings.Split(diff, "\n") { + output := diffLine + switch { + case bold(diffLine): + output = utils.Bold(diffLine) + case green(diffLine): + output = utils.Green(diffLine) + case red(diffLine): + output = utils.Red(diffLine) } - case "never": - out := cmd.OutOrStdout() - fmt.Fprintf(out, diff) - default: - return fmt.Errorf("did not understand color setting %q", color) + + fmt.Fprintln(out, output) } return nil @@ -138,3 +138,7 @@ func red(dl string) bool { func green(dl string) bool { return strings.HasPrefix(dl, "+") } + +func validColor(c string) bool { + return c == "auto" || c == "always" || c == "never" +} From 51b421207054e39c286d0a2d4db0c4b38e8550d8 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:39:54 -0500 Subject: [PATCH 35/49] add basic tests --- command/pr_diff.go | 16 +++++----- command/pr_diff_test.go | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 command/pr_diff_test.go diff --git a/command/pr_diff.go b/command/pr_diff.go index 622b50edd..f311fcc7a 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -25,6 +25,14 @@ func init() { } func prDiff(cmd *cobra.Command, args []string) error { + color, err := cmd.Flags().GetString("color") + if err != nil { + return err + } + if !validColor(color) { + return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) + } + ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) if err != nil { @@ -78,14 +86,6 @@ func prDiff(cmd *cobra.Command, args []string) error { return err } - color, err := cmd.Flags().GetString("color") - if err != nil { - return err - } - if !validColor(color) { - return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) - } - out := cmd.OutOrStdout() if color == "auto" { color = "never" diff --git a/command/pr_diff_test.go b/command/pr_diff_test.go new file mode 100644 index 000000000..a3705dad6 --- /dev/null +++ b/command/pr_diff_test.go @@ -0,0 +1,67 @@ +package command + +import ( + "bytes" + "testing" +) + +func TestPRDiff_validation(t *testing.T) { + _, err := RunCommand("pr diff --color=doublerainbow") + if err == nil { + t.Fatal("expected error") + } + eq(t, err.Error(), `did not understand color: "doublerainbow". Expected one of always, never, or auto`) +} + +func TestPRDiff(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(testDiff)) + output, err := RunCommand("pr diff") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + eq(t, testDiff, output.String()) +} + +const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml +index 73974448..b7fc0154 100644 +--- a/.github/workflows/releases.yml ++++ b/.github/workflows/releases.yml +@@ -44,6 +44,11 @@ jobs: + token: ${{secrets.SITE_GITHUB_TOKEN}} + - name: Publish documentation site + if: "!contains(github.ref, '-')" # skip prereleases ++ env: ++ GIT_COMMITTER_NAME: cli automation ++ GIT_AUTHOR_NAME: cli automation ++ GIT_COMMITTER_EMAIL: noreply@github.com ++ GIT_AUTHOR_EMAIL: noreply@github.com + run: make site-publish + - name: Move project cards + if: "!contains(github.ref, '-')" # skip prereleases +diff --git a/Makefile b/Makefile +index f2b4805c..3d7bd0f9 100644 +--- a/Makefile ++++ b/Makefile +@@ -22,8 +22,8 @@ test: + go test ./... + .PHONY: test + +-site: +- git clone https://github.com/github/cli.github.com.git "$@" ++site: bin/gh ++ bin/gh repo clone github/cli.github.com "$@" + + site-docs: site + git -C site pull +` From ccda5ced5136b50f18a336a20d1f0727d5b2737c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 18 May 2020 17:45:47 -0500 Subject: [PATCH 36/49] lint --- command/pr_diff.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index f311fcc7a..a6dccdd12 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -66,14 +66,8 @@ func prDiff(cmd *cobra.Command, args []string) error { } } - var pr *api.PullRequest - if prNum > 0 { - pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNum) - if err != nil { - return fmt.Errorf("could not find pull request: %w", err) - } - } else { - pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if prNum < 1 { + pr, err := api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) if err != nil { return fmt.Errorf("could not find pull request: %w", err) } From 821324c2fb3b833750cd861bc98659804e2c0deb Mon Sep 17 00:00:00 2001 From: dulltz Date: Tue, 19 May 2020 12:03:56 +0900 Subject: [PATCH 37/49] cosmetic - Align the first letter to uppercase. - Align with no period at the end. --- command/config.go | 4 ++-- command/issue.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/command/config.go b/command/config.go index 0350e528b..7e2a541f7 100644 --- a/command/config.go +++ b/command/config.go @@ -32,7 +32,7 @@ Current respected settings: var configGetCmd = &cobra.Command{ Use: "get ", - Short: "Prints the value of a given configuration key.", + Short: "Prints the value of a given configuration key", Long: `Get the value for a given configuration key. Examples: @@ -45,7 +45,7 @@ Examples: var configSetCmd = &cobra.Command{ Use: "set ", - Short: "Updates configuration with the value of a given key.", + Short: "Updates configuration with the value of a given key", Long: `Update the configuration by setting a key to a value. Examples: diff --git a/command/issue.go b/command/issue.go index 3c7727e53..3d3f01708 100644 --- a/command/issue.go +++ b/command/issue.go @@ -88,13 +88,13 @@ With '--web', open the issue in a web browser instead.`, } var issueCloseCmd = &cobra.Command{ Use: "close { | }", - Short: "close issue", + Short: "Close issue", Args: cobra.ExactArgs(1), RunE: issueClose, } var issueReopenCmd = &cobra.Command{ Use: "reopen { | }", - Short: "reopen issue", + Short: "Reopen issue", Args: cobra.ExactArgs(1), RunE: issueReopen, } From e13fc2465f4af332021ebc81f399dc1f447784fe Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 19 May 2020 12:21:17 -0400 Subject: [PATCH 38/49] use consistent var declaration format --- cmd/gen-docs/main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index a878e339f..c7bb84afa 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -14,10 +14,10 @@ func main() { var flagError pflag.ErrorHandling docCmd := pflag.NewFlagSet("", flagError) - var manPage = docCmd.BoolP("man-page", "", false, "Generate manual pages") - var website = docCmd.BoolP("website", "", false, "Generate website pages") - var dir = docCmd.StringP("doc-path", "", "", "Path directory where you want generate doc files") - var help = docCmd.BoolP("help", "h", false, "Help about any command") + manPage := docCmd.BoolP("man-page", "", false, "Generate manual pages") + website := docCmd.BoolP("website", "", false, "Generate website pages") + dir := docCmd.StringP("doc-path", "", "", "Path directory where you want generate doc files") + help := docCmd.BoolP("help", "h", false, "Help about any command") if err := docCmd.Parse(os.Args); err != nil { os.Exit(1) From c98b0924dcf03fcb465750e3ab9b859c9240f524 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 19 May 2020 11:58:49 -0500 Subject: [PATCH 39/49] properly handle REST errors --- api/queries_pr.go | 12 +++++++++++- command/pr_diff.go | 2 +- command/pr_diff_test.go | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 80428ae07..26c88af01 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "fmt" "io/ioutil" "net/http" @@ -224,7 +225,16 @@ func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNum int) (string, e return "", err } - return string(b), nil + if resp.StatusCode == 200 { + return string(b), nil + } + + if resp.StatusCode == 404 { + return "", &NotFoundError{errors.New("pull request not found")} + } + + return "", errors.New("pull request diff lookup failed") + } func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { diff --git a/command/pr_diff.go b/command/pr_diff.go index a6dccdd12..6f48716da 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -77,7 +77,7 @@ func prDiff(cmd *cobra.Command, args []string) error { diff, err := apiClient.PullRequestDiff(baseRepo, prNum) if err != nil { - return err + return fmt.Errorf("could not find pull request diff: %w", err) } out := cmd.OutOrStdout() diff --git a/command/pr_diff_test.go b/command/pr_diff_test.go index a3705dad6..ab883dcab 100644 --- a/command/pr_diff_test.go +++ b/command/pr_diff_test.go @@ -13,6 +13,38 @@ func TestPRDiff_validation(t *testing.T) { eq(t, err.Error(), `did not understand color: "doublerainbow". Expected one of always, never, or auto`) } +func TestPRDiff_no_current_pr(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(testDiff)) + _, err := RunCommand("pr diff") + if err == nil { + t.Fatal("expected error", err) + } + eq(t, err.Error(), `could not find pull request: no open pull requests found for branch "master"`) +} + +func TestPRDiff_argument_not_found(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(404, bytes.NewBufferString("")) + _, err := RunCommand("pr diff 123") + if err == nil { + t.Fatal("expected error", err) + } + eq(t, err.Error(), `could not find pull request diff: pull request not found`) +} + func TestPRDiff(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() @@ -30,7 +62,7 @@ func TestPRDiff(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %s", err) } - eq(t, testDiff, output.String()) + eq(t, output.String(), testDiff) } const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml From d209c0be010a203c37ee5b3f71a5e0c2b5c8a469 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 19 May 2020 11:04:43 -0700 Subject: [PATCH 40/49] Allow pr urls --- command/pr.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index cf3eaa8f7..83ddf7d13 100644 --- a/command/pr.go +++ b/command/pr.go @@ -571,7 +571,15 @@ func prReady(cmd *cobra.Command, args []string) error { var pr *api.PullRequest if len(args) > 0 { - pr, err = prFromArg(apiClient, baseRepo, args[0]) + var prNumber string + n, _ := prFromURL(args[0]) + if n != "" { + prNumber = n + } else { + prNumber = args[0] + } + + pr, err = prFromArg(apiClient, baseRepo, prNumber) if err != nil { return err } From 983a1d9c3c6690503c604389dfe5dca728d70561 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 19 May 2020 14:08:20 -0500 Subject: [PATCH 41/49] better names --- command/pr_diff.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/command/pr_diff.go b/command/pr_diff.go index 6f48716da..6e4bb4bdb 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -29,7 +29,7 @@ func prDiff(cmd *cobra.Command, args []string) error { if err != nil { return err } - if !validColor(color) { + if !validColorFlag(color) { return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) } @@ -101,11 +101,11 @@ func prDiff(cmd *cobra.Command, args []string) error { for _, diffLine := range strings.Split(diff, "\n") { output := diffLine switch { - case bold(diffLine): + case isHeaderLine(diffLine): output = utils.Bold(diffLine) - case green(diffLine): + case isAdditionLine(diffLine): output = utils.Green(diffLine) - case red(diffLine): + case isRemovalLine(diffLine): output = utils.Red(diffLine) } @@ -115,7 +115,7 @@ func prDiff(cmd *cobra.Command, args []string) error { return nil } -func bold(dl string) bool { +func isHeaderLine(dl string) bool { prefixes := []string{"+++", "---", "diff", "index"} for _, p := range prefixes { if strings.HasPrefix(dl, p) { @@ -125,14 +125,14 @@ func bold(dl string) bool { return false } -func red(dl string) bool { - return strings.HasPrefix(dl, "-") -} - -func green(dl string) bool { +func isAdditionLine(dl string) bool { return strings.HasPrefix(dl, "+") } -func validColor(c string) bool { +func isRemovalLine(dl string) bool { + return strings.HasPrefix(dl, "-") +} + +func validColorFlag(c string) bool { return c == "auto" || c == "always" || c == "never" } From dffb55889c35fc5ff5d08af6ac67a15df2fd29d7 Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 19 May 2020 15:58:27 -0400 Subject: [PATCH 42/49] install manpages for homebrew --- .goreleaser.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.goreleaser.yml b/.goreleaser.yml index 4ceeee571..be3dc2274 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -61,6 +61,7 @@ brews: install: | system "make" if build.head? bin.install "bin/gh" + man1.install Dir["manpages/*.1"] (bash_completion/"gh.sh").write `#{bin}/gh completion -s bash` (zsh_completion/"_gh").write `#{bin}/gh completion -s zsh` (fish_completion/"gh.fish").write `#{bin}/gh completion -s fish` From db2fac93ea88a6a7801185e828a63d3bb4aa8c6b Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 19 May 2020 18:43:26 -0400 Subject: [PATCH 43/49] generate manpages into ./share/man/man1 --- .gitignore | 3 +-- .goreleaser.yml | 6 +++--- Makefile | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 80891986e..00a5bb5a6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ /bin +/share/man/man1 /gh-cli .envrc /dist @@ -16,5 +17,3 @@ .DS_Store vendor/ - -manpage diff --git a/.goreleaser.yml b/.goreleaser.yml index be3dc2274..0996d24d5 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -36,7 +36,7 @@ archives: darwin: macOS format: tar.gz files: - - manpage/gh*.1 + - ./share/man/man1/gh*.1 - id: windows builds: [windows] <<: *archive_defaults @@ -61,7 +61,7 @@ brews: install: | system "make" if build.head? bin.install "bin/gh" - man1.install Dir["manpages/*.1"] + man1.install Dir["./share/man/man1/gh*.1"] (bash_completion/"gh.sh").write `#{bin}/gh completion -s bash` (zsh_completion/"_gh").write `#{bin}/gh completion -s zsh` (fish_completion/"gh.fish").write `#{bin}/gh completion -s fish` @@ -80,7 +80,7 @@ nfpms: - deb - rpm files: - "./manpage/gh*.1": "/usr/share/man/man1" + "./share/man/man1/gh*.1": "/usr/share/man/man1" scoop: bucket: diff --git a/Makefile b/Makefile index 87ec9d8d5..bcf564947 100644 --- a/Makefile +++ b/Makefile @@ -46,9 +46,9 @@ endif .PHONY: site-publish -manpage: +share/man/man1/: mkdir -p $@ .PHONY: manpages manpages: manpage - go run ./cmd/gen-docs --man-page --doc-path ./manpage + go run ./cmd/gen-docs --man-page --doc-path ./share/man/man1/ From 6387078532c2596a4c5615e079df82144801f928 Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 19 May 2020 22:40:14 -0400 Subject: [PATCH 44/49] add make manpages hook and fix makefile --- .goreleaser.yml | 4 ++++ Makefile | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 0996d24d5..a37a5c0e7 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -17,10 +17,14 @@ builds: id: macos goos: [darwin] goarch: [amd64] + hooks: + pre: make manpages + - <<: *build_defaults id: linux goos: [linux] goarch: [386, amd64, arm64] + - <<: *build_defaults id: windows goos: [windows] diff --git a/Makefile b/Makefile index bcf564947..c284f6c57 100644 --- a/Makefile +++ b/Makefile @@ -46,9 +46,9 @@ endif .PHONY: site-publish -share/man/man1/: +share/man/man1: mkdir -p $@ .PHONY: manpages -manpages: manpage +manpages: share/man/man1 go run ./cmd/gen-docs --man-page --doc-path ./share/man/man1/ From af93bab887cde7cafd3ad9d6d94e9c6c301c6af0 Mon Sep 17 00:00:00 2001 From: Mark Furland Date: Tue, 19 May 2020 22:42:44 -0400 Subject: [PATCH 45/49] remove unnecessary mkdir from makefile --- Makefile | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Makefile b/Makefile index c284f6c57..dd863cb0d 100644 --- a/Makefile +++ b/Makefile @@ -46,9 +46,6 @@ endif .PHONY: site-publish -share/man/man1: - mkdir -p $@ - .PHONY: manpages -manpages: share/man/man1 +manpages: go run ./cmd/gen-docs --man-page --doc-path ./share/man/man1/ From a9e83dcc36caefbcee47a9048893ce90437db1eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 13:07:48 +0200 Subject: [PATCH 46/49] Tweak release process re: man pages --- .goreleaser.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index a37a5c0e7..6240b028f 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -6,6 +6,7 @@ release: before: hooks: - go mod tidy + - make manpages builds: - <<: &build_defaults @@ -17,8 +18,6 @@ builds: id: macos goos: [darwin] goarch: [amd64] - hooks: - pre: make manpages - <<: *build_defaults id: linux @@ -63,7 +62,7 @@ brews: depends_on "go" end install: | - system "make" if build.head? + system "make", "bin/gh", "manpages" if build.head? bin.install "bin/gh" man1.install Dir["./share/man/man1/gh*.1"] (bash_completion/"gh.sh").write `#{bin}/gh completion -s bash` From 6385c32031874d8f2c968c9d05e873b167af0ca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 13:15:44 +0200 Subject: [PATCH 47/49] Include license information in release archives --- .goreleaser.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.goreleaser.yml b/.goreleaser.yml index 6240b028f..ceb22c12e 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -39,12 +39,15 @@ archives: darwin: macOS format: tar.gz files: + - LICENSE - ./share/man/man1/gh*.1 - id: windows builds: [windows] <<: *archive_defaults wrap_in_directory: false format: zip + files: + - LICENSE brews: - name: gh From fb63efcf0520102315e3450d576224e6f71a2cdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 14:59:40 +0200 Subject: [PATCH 48/49] Avoid crash around "DISMISSED" or "PENDING" reviewer states --- command/pr.go | 15 +++++++++++++-- command/pr_test.go | 2 +- .../prViewPreviewWithReviewersByNumber.json | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/command/pr.go b/command/pr.go index 83ddf7d13..3bf36b432 100644 --- a/command/pr.go +++ b/command/pr.go @@ -623,6 +623,8 @@ const ( approvedReviewState = "APPROVED" changesRequestedReviewState = "CHANGES_REQUESTED" commentedReviewState = "COMMENTED" + dismissedReviewState = "DISMISSED" + pendingReviewState = "PENDING" ) type reviewerState struct { @@ -648,8 +650,14 @@ func colorFuncForReviewerState(state string) func(string) string { // formattedReviewerState formats a reviewerState with state color func formattedReviewerState(reviewer *reviewerState) string { - stateColorFunc := colorFuncForReviewerState(reviewer.State) - return fmt.Sprintf("%s (%s)", reviewer.Name, stateColorFunc(strings.ReplaceAll(strings.Title(strings.ToLower(reviewer.State)), "_", " "))) + state := reviewer.State + if state == dismissedReviewState { + // Show "DISMISSED" review as "COMMENTED", since "dimissed" only makes + // sense when displayed in an events timeline but not in the final tally. + state = commentedReviewState + } + stateColorFunc := colorFuncForReviewerState(state) + return fmt.Sprintf("%s (%s)", reviewer.Name, stateColorFunc(strings.ReplaceAll(strings.Title(strings.ToLower(state)), "_", " "))) } // prReviewerList generates a reviewer list with their last state @@ -705,6 +713,9 @@ func parseReviewers(pr api.PullRequest) []*reviewerState { // Convert map to slice for ease of sort result := make([]*reviewerState, 0, len(reviewerStates)) for _, reviewer := range reviewerStates { + if reviewer.State == pendingReviewState { + continue + } result = append(result, reviewer) } diff --git a/command/pr_test.go b/command/pr_test.go index 24fce9d63..22f7102f9 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -477,7 +477,7 @@ func TestPRView_Preview(t *testing.T) { fixture: "../test/fixtures/prViewPreviewWithReviewersByNumber.json", expectedOutputs: []string{ `Blueberries are from a fork`, - `Reviewers: DEF \(Commented\), def \(Changes requested\), ghost \(Approved\), xyz \(Approved\), 123 \(Requested\), Team 1 \(Requested\), abc \(Requested\)\n`, + `Reviewers: DEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), xyz \(Approved\), 123 \(Requested\), Team 1 \(Requested\), abc \(Requested\)\n`, `blueberries taste good`, `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12\n`, }, diff --git a/test/fixtures/prViewPreviewWithReviewersByNumber.json b/test/fixtures/prViewPreviewWithReviewersByNumber.json index 791512d41..4d2bd57af 100644 --- a/test/fixtures/prViewPreviewWithReviewersByNumber.json +++ b/test/fixtures/prViewPreviewWithReviewersByNumber.json @@ -70,6 +70,24 @@ "login": "" }, "state": "APPROVED" + }, + { + "author": { + "login": "hubot" + }, + "state": "CHANGES_REQUESTED" + }, + { + "author": { + "login": "hubot" + }, + "state": "DISMISSED" + }, + { + "author": { + "login": "monalisa" + }, + "state": "PENDING" } ] }, From e5b78d3342c4de75d67f449861bb332bc03f5bd6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 20 May 2020 11:21:05 -0700 Subject: [PATCH 49/49] Fix lint errors --- cmd/gen-docs/main.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index c7bb84afa..92581d07a 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -31,11 +31,10 @@ func main() { os.Exit(1) } - if(*dir == "") { + if *dir == "" { fatal("no dir set") } - err := os.MkdirAll(*dir, 0755) if err != nil { fatal(err) @@ -48,13 +47,12 @@ func main() { } } - if *manPage { header := &doc.GenManHeader{ Title: "gh", Section: "1", - Source: "", //source and manual are just put at the top of the manpage, before name - Manual: "", //if source is an empty string, it's set to "Auto generated by spf13/cobra" + Source: "", //source and manual are just put at the top of the manpage, before name + Manual: "", //if source is an empty string, it's set to "Auto generated by spf13/cobra" } err = doc.GenManTree(command.RootCmd, header, *dir) if err != nil {