From 6fde02df1c6881c4f2cd98b95742af0cb413649b Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 9 Nov 2020 14:24:19 -0800 Subject: [PATCH 1/7] use named output param --- pkg/cmd/pr/create/create.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index f8d67cb15..bbfaf7c8c 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -127,7 +127,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co return cmd } -func createRun(opts *CreateOptions) error { +func createRun(opts *CreateOptions) (err error) { cs := opts.IO.ColorScheme() httpClient, err := opts.HttpClient() @@ -153,7 +153,6 @@ func createRun(opts *CreateOptions) error { } else { // TODO: if RepoNetwork is going to be requested anyway in `repoContext.HeadRepos()`, // consider piggybacking on that result instead of performing a separate lookup - var err error baseRepo, err = api.GitHubRepo(client, br) if err != nil { return err From 6671106448ed7b342797b426e15af032dfe3b1be Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 9 Nov 2020 17:15:21 -0800 Subject: [PATCH 2/7] WIP works, probably some title/body input edge cases --- pkg/cmd/issue/create/create.go | 97 ++-- pkg/cmd/pr/create/create.go | 753 ++++++++++++++----------- pkg/cmd/pr/shared/params.go | 26 +- pkg/cmd/pr/shared/survey.go | 404 +++++++++++++ pkg/cmd/pr/shared/title_body_survey.go | 396 ------------- pkg/githubtemplate/github_template.go | 1 + 6 files changed, 898 insertions(+), 779 deletions(-) create mode 100644 pkg/cmd/pr/shared/survey.go delete mode 100644 pkg/cmd/pr/shared/title_body_survey.go diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 2a8ce27c6..2d5e86898 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -7,12 +7,10 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" - "github.com/cli/cli/git" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" prShared "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/pkg/githubtemplate" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -101,14 +99,7 @@ func createRun(opts *CreateOptions) error { return err } - var nonLegacyTemplateFiles []string - if opts.RootDirOverride != "" { - nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(opts.RootDirOverride, "ISSUE_TEMPLATE") - } else if opts.RepoOverride == "" { - if rootDir, err := git.ToplevelDir(); err == nil { - nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(rootDir, "ISSUE_TEMPLATE") - } - } + templateFiles, legacyTemplate := prShared.FindTemplates(opts.RootDirOverride, "ISSUE_TEMPLATE") isTerminal := opts.IO.IsStdoutTTY() @@ -117,14 +108,24 @@ func createRun(opts *CreateOptions) error { milestones = []string{opts.Milestone} } + tb := prShared.IssueMetadataState{ + Type: prShared.IssueMetadata, + Assignees: opts.Assignees, + Labels: opts.Labels, + Projects: opts.Projects, + Milestones: milestones, + Title: opts.Title, + Body: opts.Body, + } + if opts.WebMode { openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") if opts.Title != "" || opts.Body != "" { - openURL, err = prShared.WithPrAndIssueQueryParams(openURL, opts.Title, opts.Body, opts.Assignees, opts.Labels, opts.Projects, milestones) + openURL, err = prShared.WithPrAndIssueQueryParams(openURL, tb) if err != nil { return err } - } else if len(nonLegacyTemplateFiles) > 1 { + } else if len(templateFiles) > 1 { openURL += "/choose" } if isTerminal { @@ -146,59 +147,65 @@ func createRun(opts *CreateOptions) error { } action := prShared.SubmitAction - tb := prShared.IssueMetadataState{ - Type: prShared.IssueMetadata, - Assignees: opts.Assignees, - Labels: opts.Labels, - Projects: opts.Projects, - Milestones: milestones, - } - - title := opts.Title - body := opts.Body if opts.Interactive { - var legacyTemplateFile *string - if opts.RepoOverride == "" { - if rootDir, err := git.ToplevelDir(); err == nil { - // TODO: figure out how to stub this in tests - legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "ISSUE_TEMPLATE") - } - } - editorCommand, err := cmdutil.DetermineEditor(opts.Config) if err != nil { return err } - err = prShared.TitleBodySurvey(opts.IO, editorCommand, &tb, apiClient, baseRepo, title, body, prShared.Defaults{}, nonLegacyTemplateFiles, legacyTemplateFile, false, repo.ViewerCanTriage()) + err = prShared.TitleSurvey(&tb) if err != nil { - return fmt.Errorf("could not collect title and/or body: %w", err) + return err } - action = tb.Action + templateContent := "" - if tb.Action == prShared.CancelAction { + templateContent, err = prShared.TemplateSurvey(templateFiles, legacyTemplate, tb) + if err != nil { + return err + } + + err = prShared.BodySurvey(&tb, templateContent, editorCommand) + if err != nil { + return err + } + + if tb.Body == "" { + tb.Body = templateContent + } + + action, err := prShared.ConfirmSubmission(!tb.HasMetadata(), repo.ViewerCanTriage()) + if err != nil { + return fmt.Errorf("unable to confirm: %w", err) + } + + if action == prShared.MetadataAction { + err = prShared.MetadataSurvey(opts.IO, apiClient, baseRepo, &tb) + if err != nil { + return err + } + + action, err = prShared.ConfirmSubmission(!tb.HasMetadata(), false) + if err != nil { + return err + } + } + + if action == prShared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") return nil } - - if title == "" { - title = tb.Title - } - if body == "" { - body = tb.Body - } } else { - if title == "" { + if tb.Title == "" { return fmt.Errorf("title can't be blank") } } if action == prShared.PreviewAction { openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") - openURL, err = prShared.WithPrAndIssueQueryParams(openURL, title, body, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) + openURL, err = prShared.WithPrAndIssueQueryParams(openURL, tb) if err != nil { return err } @@ -208,8 +215,8 @@ func createRun(opts *CreateOptions) error { return utils.OpenInBrowser(openURL) } else if action == prShared.SubmitAction { params := map[string]interface{}{ - "title": title, - "body": body, + "title": tb.Title, + "body": tb.Body, } err = prShared.AddMetadataToIssueParams(apiClient, baseRepo, params, &tb) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index bbfaf7c8c..32bd5807d 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -18,7 +18,6 @@ import ( "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/pkg/githubtemplate" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/utils" @@ -26,6 +25,7 @@ import ( ) type CreateOptions struct { + // This struct stores user input and factory functions HttpClient func() (*http.Client, error) Config func() (config.Config, error) IO *iostreams.IOStreams @@ -56,6 +56,20 @@ type CreateOptions struct { Milestone string } +type CreateContext struct { + // This struct stores contextual data about the creation process and is for building up enough + // data to create a pull request + RepoContext *context.ResolvedRemotes + BaseRepo *api.Repository + HeadRepo ghrepo.Interface + BaseTrackingBranch string + BaseBranch string + HeadBranch string + HeadBranchLabel string + HeadRemote *context.Remote + IsPushEnabled bool +} + func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command { opts := &CreateOptions{ IO: f.IOStreams, @@ -92,10 +106,16 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.Interactive = !(opts.TitleProvided && opts.BodyProvided) + // TODO check on edge cases around title/body provision + if !opts.IO.CanPrompt() && !opts.WebMode && !opts.TitleProvided && !opts.Autofill { return &cmdutil.FlagError{Err: errors.New("--title or --fill required when not running interactively")} } + if !opts.IO.CanPrompt() { + opts.Interactive = false + } + if opts.IsDraft && opts.WebMode { return errors.New("the --draft flag is not supported with --web") } @@ -128,148 +148,20 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) (err error) { - cs := opts.IO.ColorScheme() - httpClient, err := opts.HttpClient() if err != nil { return err } client := api.NewClientFromHTTP(httpClient) - remotes, err := opts.Remotes() + ctx, err := NewCreateContext(opts) if err != nil { return err } - repoContext, err := context.ResolveRemotesToRepos(remotes, client, opts.RepoOverride) - if err != nil { - return err - } - - var baseRepo *api.Repository - if br, err := repoContext.BaseRepo(opts.IO); err == nil { - if r, ok := br.(*api.Repository); ok { - baseRepo = r - } else { - // TODO: if RepoNetwork is going to be requested anyway in `repoContext.HeadRepos()`, - // consider piggybacking on that result instead of performing a separate lookup - baseRepo, err = api.GitHubRepo(client, br) - if err != nil { - return err - } - } - } else { - return fmt.Errorf("could not determine base repository: %w", err) - } - - isPushEnabled := false - headBranch := opts.HeadBranch - headBranchLabel := opts.HeadBranch - if headBranch == "" { - headBranch, err = opts.Branch() - if err != nil { - return fmt.Errorf("could not determine the current branch: %w", err) - } - headBranchLabel = headBranch - isPushEnabled = true - } else if idx := strings.IndexRune(headBranch, ':'); idx >= 0 { - headBranch = headBranch[idx+1:] - } - - if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { - fmt.Fprintf(opts.IO.ErrOut, "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) - } - - var headRepo ghrepo.Interface - var headRemote *context.Remote - - if isPushEnabled { - // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(remotes, headBranch); pushedTo != nil { - isPushEnabled = false - if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { - headRepo = r - headRemote = r - headBranchLabel = pushedTo.BranchName - if !ghrepo.IsSame(baseRepo, headRepo) { - headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), pushedTo.BranchName) - } - } - } - } - - // otherwise, ask the user for the head repository using info obtained from the API - if headRepo == nil && isPushEnabled && opts.IO.CanPrompt() { - pushableRepos, err := repoContext.HeadRepos() - if err != nil { - return err - } - - if len(pushableRepos) == 0 { - pushableRepos, err = api.RepoFindForks(client, baseRepo, 3) - if err != nil { - return err - } - } - - currentLogin, err := api.CurrentLoginName(client, baseRepo.RepoHost()) - if err != nil { - return err - } - - hasOwnFork := false - var pushOptions []string - for _, r := range pushableRepos { - pushOptions = append(pushOptions, ghrepo.FullName(r)) - if r.RepoOwner() == currentLogin { - hasOwnFork = true - } - } - - if !hasOwnFork { - pushOptions = append(pushOptions, "Create a fork of "+ghrepo.FullName(baseRepo)) - } - pushOptions = append(pushOptions, "Skip pushing the branch") - pushOptions = append(pushOptions, "Cancel") - - var selectedOption int - err = prompt.SurveyAskOne(&survey.Select{ - Message: fmt.Sprintf("Where should we push the '%s' branch?", headBranch), - Options: pushOptions, - }, &selectedOption) - if err != nil { - return err - } - - if selectedOption < len(pushableRepos) { - headRepo = pushableRepos[selectedOption] - if !ghrepo.IsSame(baseRepo, headRepo) { - headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) - } - } else if pushOptions[selectedOption] == "Skip pushing the branch" { - isPushEnabled = false - } else if pushOptions[selectedOption] == "Cancel" { - return cmdutil.SilentError - } else { - // "Create a fork of ..." - if baseRepo.IsPrivate { - return fmt.Errorf("cannot fork private repository %s", ghrepo.FullName(baseRepo)) - } - headBranchLabel = fmt.Sprintf("%s:%s", currentLogin, headBranch) - } - } - - if headRepo == nil && isPushEnabled && !opts.IO.CanPrompt() { - fmt.Fprintf(opts.IO.ErrOut, "aborted: you must first push the current branch to a remote, or use the --head flag") - return cmdutil.SilentError - } - - baseBranch := opts.BaseBranch - if baseBranch == "" { - baseBranch = baseRepo.DefaultBranchRef.Name - } - if headBranch == baseBranch && headRepo != nil && ghrepo.IsSame(baseRepo, headRepo) { - return fmt.Errorf("must be on a branch named differently than %q", baseBranch) + defs, defaultsErr := computeDefaults(*ctx) + if defaultsErr != nil && (opts.Autofill || opts.WebMode || !opts.Interactive) { + return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) } var milestoneTitles []string @@ -277,64 +169,9 @@ func createRun(opts *CreateOptions) (err error) { milestoneTitles = []string{opts.Milestone} } - baseTrackingBranch := baseBranch - if baseRemote, err := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()); err == nil { - baseTrackingBranch = fmt.Sprintf("%s/%s", baseRemote.Name, baseBranch) - } - defs, defaultsErr := computeDefaults(baseTrackingBranch, headBranch) - - title := opts.Title - body := opts.Body - - action := shared.SubmitAction - if opts.WebMode { - action = shared.PreviewAction - if (title == "" || body == "") && defaultsErr != nil { - return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) - } - } else if opts.Autofill { - if defaultsErr != nil && !(opts.TitleProvided || opts.BodyProvided) { - return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) - } - if !opts.TitleProvided { - title = defs.Title - } - if !opts.BodyProvided { - body = defs.Body - } - } - - if !opts.WebMode { - existingPR, err := api.PullRequestForBranch(client, baseRepo, baseBranch, headBranchLabel, []string{"OPEN"}) - var notFound *api.NotFoundError - if err != nil && !errors.As(err, ¬Found) { - return fmt.Errorf("error checking for existing pull request: %w", err) - } - if err == nil { - return fmt.Errorf("a pull request for branch %q into branch %q already exists:\n%s", headBranchLabel, baseBranch, existingPR.URL) - } - } - - isTerminal := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - - if !opts.WebMode && !opts.Autofill { - message := "\nCreating pull request for %s into %s in %s\n\n" - if opts.IsDraft { - message = "\nCreating draft pull request for %s into %s in %s\n\n" - } - - if isTerminal { - fmt.Fprintf(opts.IO.ErrOut, message, - cs.Cyan(headBranchLabel), - cs.Cyan(baseBranch), - ghrepo.FullName(baseRepo)) - if (title == "" || body == "") && defaultsErr != nil { - fmt.Fprintf(opts.IO.ErrOut, "%s warning: could not compute title or body defaults: %s\n", cs.Yellow("!"), defaultsErr) - } - } - } - - tb := shared.IssueMetadataState{ + state := shared.IssueMetadataState{ + Title: defs.Title, + Body: defs.Body, Type: shared.PRMetadata, Reviewers: opts.Reviewers, Assignees: opts.Assignees, @@ -343,162 +180,126 @@ func createRun(opts *CreateOptions) (err error) { Milestones: milestoneTitles, } - if !opts.WebMode && !opts.Autofill && opts.Interactive { - var nonLegacyTemplateFiles []string - var legacyTemplateFile *string + if opts.TitleProvided { + state.Title = opts.Title + } - if opts.RootDirOverride != "" { - nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(opts.RootDirOverride, "PULL_REQUEST_TEMPLATE") - legacyTemplateFile = githubtemplate.FindLegacy(opts.RootDirOverride, "PULL_REQUEST_TEMPLATE") - } else if rootDir, err := git.ToplevelDir(); err == nil { - nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(rootDir, "PULL_REQUEST_TEMPLATE") - legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "PULL_REQUEST_TEMPLATE") + if opts.BodyProvided { + state.Body = opts.Body + } + + if opts.WebMode { + err := handlePush(*opts, *ctx) + if err != nil { + return err } + return previewPR(*opts, *ctx, state) + } - editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if opts.Autofill || !opts.Interactive { + return submitPR(*opts, *ctx, state) + } + + existingPR, err := api.PullRequestForBranch( + client, ctx.BaseRepo, ctx.BaseBranch, ctx.HeadBranchLabel, []string{"OPEN"}) + var notFound *api.NotFoundError + if err != nil && !errors.As(err, ¬Found) { + return fmt.Errorf("error checking for existing pull request: %w", err) + } + if err == nil { + return fmt.Errorf("a pull request for branch %q into branch %q already exists:\n%s", + ctx.HeadBranchLabel, ctx.BaseBranch, existingPR.URL) + } + + message := "\nCreating pull request for %s into %s in %s\n\n" + if opts.IsDraft { + message = "\nCreating draft pull request for %s into %s in %s\n\n" + } + + cs := opts.IO.ColorScheme() + + fmt.Fprintf(opts.IO.ErrOut, message, + cs.Cyan(ctx.HeadBranchLabel), + cs.Cyan(ctx.BaseBranch), + ghrepo.FullName(ctx.BaseRepo)) + if (state.Title == "" || state.Body == "") && defaultsErr != nil { + fmt.Fprintf(opts.IO.ErrOut, + "%s warning: could not compute title or body defaults: %s\n", cs.Yellow("!"), defaultsErr) + } + + if !opts.TitleProvided { + err = shared.TitleSurvey(&state) + if err != nil { + return err + } + } + + editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + + templateContent := "" + if !opts.BodyProvided { + templateFiles, legacyTemplate := shared.FindTemplates(opts.RootDirOverride, "PULL_REQUEST_TEMPLATE") + + templateContent, err = shared.TemplateSurvey(templateFiles, legacyTemplate, state) if err != nil { return err } - err = shared.TitleBodySurvey(opts.IO, editorCommand, &tb, client, baseRepo, title, body, defs, nonLegacyTemplateFiles, legacyTemplateFile, true, baseRepo.ViewerCanTriage()) - if err != nil { - return fmt.Errorf("could not collect title and/or body: %w", err) - } - - action = tb.Action - - if action == shared.CancelAction { - fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - return nil - } - - if title == "" { - title = tb.Title - } - if body == "" { - body = tb.Body - } - } - - if action == shared.SubmitAction && title == "" { - return errors.New("pull request title must not be blank") - } - - didForkRepo := false - // if a head repository could not be determined so far, automatically create - // one by forking the base repository - if headRepo == nil && isPushEnabled { - headRepo, err = api.ForkRepo(client, baseRepo) - if err != nil { - return fmt.Errorf("error forking repo: %w", err) - } - didForkRepo = true - } - - if headRemote == nil && headRepo != nil { - headRemote, _ = repoContext.RemoteForRepo(headRepo) - } - - // There are two cases when an existing remote for the head repo will be - // missing: - // 1. the head repo was just created by auto-forking; - // 2. an existing fork was discovered by querying the API. - // - // In either case, we want to add the head repo as a new git remote so we - // can push to it. - if headRemote == nil && isPushEnabled { - cfg, err := opts.Config() + err = shared.BodySurvey(&state, templateContent, editorCommand) if err != nil { return err } - cloneProtocol, _ := cfg.Get(headRepo.RepoHost(), "git_protocol") - headRepoURL := ghrepo.FormatRemoteURL(headRepo, cloneProtocol) - - // TODO: prevent clashes with another remote of a same name - gitRemote, err := git.AddRemote("fork", headRepoURL) - if err != nil { - return fmt.Errorf("error adding remote: %w", err) - } - headRemote = &context.Remote{ - Remote: gitRemote, - Repo: headRepo, + if state.Body == "" { + state.Body = templateContent } } - // automatically push the branch if it hasn't been pushed anywhere yet - if isPushEnabled { - pushBranch := func() error { - pushTries := 0 - maxPushTries := 3 - for { - r := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "") - defer r.Flush() - cmdErr := r - cmdOut := opts.IO.Out - if err := git.Push(headRemote.Name, fmt.Sprintf("HEAD:%s", headBranch), cmdOut, cmdErr); err != nil { - if didForkRepo && pushTries < maxPushTries { - pushTries++ - // first wait 2 seconds after forking, then 4s, then 6s - waitSeconds := 2 * pushTries - fmt.Fprintf(opts.IO.ErrOut, "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) - time.Sleep(time.Duration(waitSeconds) * time.Second) - continue - } - return err - } - break - } - return nil - } + allowMetadata := ctx.BaseRepo.ViewerCanTriage() + action, err := shared.ConfirmSubmission(!state.HasMetadata(), allowMetadata) + if err != nil { + return fmt.Errorf("unable to confirm: %w", err) + } - err := pushBranch() + if action == shared.MetadataAction { + err = shared.MetadataSurvey(opts.IO, client, ctx.BaseRepo, &state) if err != nil { return err } + + action, err = shared.ConfirmSubmission(!state.HasMetadata(), false) + if err != nil { + return err + } + } + + if action == shared.CancelAction { + fmt.Fprintln(opts.IO.ErrOut, "Discarding.") + return nil + } + + err = handlePush(*opts, *ctx) + if err != nil { + return err + } + + if action == shared.PreviewAction { + return previewPR(*opts, *ctx, state) } if action == shared.SubmitAction { - params := map[string]interface{}{ - "title": title, - "body": body, - "draft": opts.IsDraft, - "baseRefName": baseBranch, - "headRefName": headBranchLabel, - } - - err = shared.AddMetadataToIssueParams(client, baseRepo, params, &tb) - if err != nil { - return err - } - - pr, err := api.CreatePullRequest(client, baseRepo, params) - if pr != nil { - fmt.Fprintln(opts.IO.Out, pr.URL) - } - if err != nil { - if pr != nil { - return fmt.Errorf("pull request update failed: %w", err) - } - return fmt.Errorf("pull request create failed: %w", err) - } - } else if action == shared.PreviewAction { - openURL, err := generateCompareURL(baseRepo, baseBranch, headBranchLabel, title, body, tb.Assignees, tb.Labels, tb.Projects, tb.Milestones) - if err != nil { - return err - } - if isTerminal { - fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) - } - return utils.OpenInBrowser(openURL) - } else { - panic("Unreachable state") + return submitPR(*opts, *ctx, state) } - return nil + return errors.New("expected to cancel, preview, or submit") } -func computeDefaults(baseRef, headRef string) (shared.Defaults, error) { +func computeDefaults(createCtx CreateContext) (shared.Defaults, error) { + baseRef := createCtx.BaseTrackingBranch + headRef := createCtx.HeadBranch out := shared.Defaults{} commits, err := git.Commits(baseRef, headRef) @@ -567,9 +368,311 @@ func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.Tr return nil } -func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestones []string) (string, error) { - u := ghrepo.GenerateRepoURL(r, "compare/%s...%s?expand=1", url.QueryEscape(base), url.QueryEscape(head)) - url, err := shared.WithPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestones) +func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { + httpClient, err := opts.HttpClient() + if err != nil { + return nil, err + } + client := api.NewClientFromHTTP(httpClient) + + remotes, err := opts.Remotes() + if err != nil { + return nil, err + } + + repoContext, err := context.ResolveRemotesToRepos(remotes, client, opts.RepoOverride) + if err != nil { + return nil, err + } + + var baseRepo *api.Repository + if br, err := repoContext.BaseRepo(opts.IO); err == nil { + if r, ok := br.(*api.Repository); ok { + baseRepo = r + } else { + // TODO: if RepoNetwork is going to be requested anyway in `repoContext.HeadRepos()`, + // consider piggybacking on that result instead of performing a separate lookup + baseRepo, err = api.GitHubRepo(client, br) + if err != nil { + return nil, err + } + } + } else { + return nil, fmt.Errorf("could not determine base repository: %w", err) + } + + isPushEnabled := false + headBranch := opts.HeadBranch + headBranchLabel := opts.HeadBranch + if headBranch == "" { + headBranch, err = opts.Branch() + if err != nil { + return nil, fmt.Errorf("could not determine the current branch: %w", err) + } + headBranchLabel = headBranch + isPushEnabled = true + } else if idx := strings.IndexRune(headBranch, ':'); idx >= 0 { + headBranch = headBranch[idx+1:] + } + + if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { + fmt.Fprintf(opts.IO.ErrOut, "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) + } + + var headRepo ghrepo.Interface + var headRemote *context.Remote + + if isPushEnabled { + // determine whether the head branch is already pushed to a remote + if pushedTo := determineTrackingBranch(remotes, headBranch); pushedTo != nil { + isPushEnabled = false + if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { + headRepo = r + headRemote = r + headBranchLabel = pushedTo.BranchName + if !ghrepo.IsSame(baseRepo, headRepo) { + headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), pushedTo.BranchName) + } + } + } + } + + // otherwise, ask the user for the head repository using info obtained from the API + if headRepo == nil && isPushEnabled && opts.IO.CanPrompt() { + pushableRepos, err := repoContext.HeadRepos() + if err != nil { + return nil, err + } + + if len(pushableRepos) == 0 { + pushableRepos, err = api.RepoFindForks(client, baseRepo, 3) + if err != nil { + return nil, err + } + } + + currentLogin, err := api.CurrentLoginName(client, baseRepo.RepoHost()) + if err != nil { + return nil, err + } + + hasOwnFork := false + var pushOptions []string + for _, r := range pushableRepos { + pushOptions = append(pushOptions, ghrepo.FullName(r)) + if r.RepoOwner() == currentLogin { + hasOwnFork = true + } + } + + if !hasOwnFork { + pushOptions = append(pushOptions, "Create a fork of "+ghrepo.FullName(baseRepo)) + } + pushOptions = append(pushOptions, "Skip pushing the branch") + pushOptions = append(pushOptions, "Cancel") + + var selectedOption int + err = prompt.SurveyAskOne(&survey.Select{ + Message: fmt.Sprintf("Where should we push the '%s' branch?", headBranch), + Options: pushOptions, + }, &selectedOption) + if err != nil { + return nil, err + } + + if selectedOption < len(pushableRepos) { + headRepo = pushableRepos[selectedOption] + if !ghrepo.IsSame(baseRepo, headRepo) { + headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) + } + } else if pushOptions[selectedOption] == "Skip pushing the branch" { + isPushEnabled = false + } else if pushOptions[selectedOption] == "Cancel" { + return nil, cmdutil.SilentError + } else { + // "Create a fork of ..." + if baseRepo.IsPrivate { + return nil, fmt.Errorf("cannot fork private repository %s", ghrepo.FullName(baseRepo)) + } + headBranchLabel = fmt.Sprintf("%s:%s", currentLogin, headBranch) + } + } + + if headRepo == nil && isPushEnabled && !opts.IO.CanPrompt() { + fmt.Fprintf(opts.IO.ErrOut, "aborted: you must first push the current branch to a remote, or use the --head flag") + return nil, cmdutil.SilentError + } + + baseBranch := opts.BaseBranch + if baseBranch == "" { + baseBranch = baseRepo.DefaultBranchRef.Name + } + if headBranch == baseBranch && headRepo != nil && ghrepo.IsSame(baseRepo, headRepo) { + return nil, fmt.Errorf("must be on a branch named differently than %q", baseBranch) + } + + baseTrackingBranch := baseBranch + if baseRemote, err := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()); err == nil { + baseTrackingBranch = fmt.Sprintf("%s/%s", baseRemote.Name, baseBranch) + } + + return &CreateContext{ + BaseRepo: baseRepo, + HeadRepo: headRepo, + BaseBranch: baseBranch, + BaseTrackingBranch: baseTrackingBranch, + HeadBranch: headBranch, + HeadBranchLabel: headBranchLabel, + HeadRemote: headRemote, + IsPushEnabled: isPushEnabled, + RepoContext: repoContext, + }, nil + +} + +func submitPR(opts CreateOptions, createCtx CreateContext, state shared.IssueMetadataState) error { + httpClient, err := opts.HttpClient() + if err != nil { + return nil + } + client := api.NewClientFromHTTP(httpClient) + + params := map[string]interface{}{ + "title": state.Title, + "body": state.Body, + "draft": opts.IsDraft, + "baseRefName": createCtx.BaseBranch, + "headRefName": createCtx.HeadBranchLabel, + } + + if params["title"] == "" { + return errors.New("pull request title must not be blank") + } + + err = shared.AddMetadataToIssueParams(client, createCtx.BaseRepo, params, &state) + if err != nil { + return err + } + + pr, err := api.CreatePullRequest(client, createCtx.BaseRepo, params) + if pr != nil { + fmt.Fprintln(opts.IO.Out, pr.URL) + } + if err != nil { + if pr != nil { + return fmt.Errorf("pull request update failed: %w", err) + } + return fmt.Errorf("pull request create failed: %w", err) + } + return nil +} + +func previewPR(opts CreateOptions, createCtx CreateContext, state shared.IssueMetadataState) error { + openURL, err := generateCompareURL(createCtx, state) + if err != nil { + return err + } + + if opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) + } + return utils.OpenInBrowser(openURL) + +} + +func handlePush(opts CreateOptions, ctx CreateContext) error { + didForkRepo := false + headRepo := ctx.HeadRepo + headRemote := ctx.HeadRemote + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + client := api.NewClientFromHTTP(httpClient) + + // if a head repository could not be determined so far, automatically create + // one by forking the base repository + if headRepo == nil && ctx.IsPushEnabled { + headRepo, err = api.ForkRepo(client, ctx.BaseRepo) + if err != nil { + return fmt.Errorf("error forking repo: %w", err) + } + didForkRepo = true + } + + if headRemote == nil && headRepo != nil { + headRemote, _ = ctx.RepoContext.RemoteForRepo(headRepo) + } + + // There are two cases when an existing remote for the head repo will be + // missing: + // 1. the head repo was just created by auto-forking; + // 2. an existing fork was discovered by querying the API. + // + // In either case, we want to add the head repo as a new git remote so we + // can push to it. + if headRemote == nil && ctx.IsPushEnabled { + cfg, err := opts.Config() + if err != nil { + return err + } + cloneProtocol, _ := cfg.Get(headRepo.RepoHost(), "git_protocol") + + headRepoURL := ghrepo.FormatRemoteURL(headRepo, cloneProtocol) + + // TODO: prevent clashes with another remote of a same name + gitRemote, err := git.AddRemote("fork", headRepoURL) + if err != nil { + return fmt.Errorf("error adding remote: %w", err) + } + headRemote = &context.Remote{ + Remote: gitRemote, + Repo: headRepo, + } + } + + // automatically push the branch if it hasn't been pushed anywhere yet + if ctx.IsPushEnabled { + pushBranch := func() error { + pushTries := 0 + maxPushTries := 3 + for { + r := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "") + defer r.Flush() + cmdErr := r + cmdOut := opts.IO.Out + if err := git.Push(headRemote.Name, fmt.Sprintf("HEAD:%s", ctx.HeadBranch), cmdOut, cmdErr); err != nil { + if didForkRepo && pushTries < maxPushTries { + pushTries++ + // first wait 2 seconds after forking, then 4s, then 6s + waitSeconds := 2 * pushTries + fmt.Fprintf(opts.IO.ErrOut, "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) + time.Sleep(time.Duration(waitSeconds) * time.Second) + continue + } + return err + } + break + } + return nil + } + + err := pushBranch() + if err != nil { + return err + } + } + + return nil +} + +func generateCompareURL(createCtx CreateContext, state shared.IssueMetadataState) (string, error) { + u := ghrepo.GenerateRepoURL( + createCtx.BaseRepo, + "compare/%s...%s?expand=1", + url.QueryEscape(createCtx.BaseBranch), url.QueryEscape(createCtx.HeadBranch)) + url, err := shared.WithPrAndIssueQueryParams(u, state) if err != nil { return "", err } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 616b4b358..9edb9e9e7 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -9,29 +9,29 @@ import ( "github.com/cli/cli/internal/ghrepo" ) -func WithPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, projects []string, milestones []string) (string, error) { +func WithPrAndIssueQueryParams(baseURL string, state IssueMetadataState) (string, error) { u, err := url.Parse(baseURL) if err != nil { return "", err } q := u.Query() - if title != "" { - q.Set("title", title) + if state.Title != "" { + q.Set("title", state.Title) } - if body != "" { - q.Set("body", body) + if state.Body != "" { + q.Set("body", state.Body) } - if len(assignees) > 0 { - q.Set("assignees", strings.Join(assignees, ",")) + if len(state.Assignees) > 0 { + q.Set("assignees", strings.Join(state.Assignees, ",")) } - if len(labels) > 0 { - q.Set("labels", strings.Join(labels, ",")) + if len(state.Labels) > 0 { + q.Set("labels", strings.Join(state.Labels, ",")) } - if len(projects) > 0 { - q.Set("projects", strings.Join(projects, ",")) + if len(state.Projects) > 0 { + q.Set("projects", strings.Join(state.Projects, ",")) } - if len(milestones) > 0 { - q.Set("milestone", milestones[0]) + if len(state.Milestones) > 0 { + q.Set("milestone", state.Milestones[0]) } u.RawQuery = q.Encode() return u.String(), nil diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go new file mode 100644 index 000000000..a1a3f65fd --- /dev/null +++ b/pkg/cmd/pr/shared/survey.go @@ -0,0 +1,404 @@ +package shared + +import ( + "fmt" + "strings" + + "github.com/AlecAivazis/survey/v2" + "github.com/cli/cli/api" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/githubtemplate" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/cli/cli/pkg/surveyext" + "github.com/cli/cli/utils" +) + +type Defaults struct { + Title string + Body string +} + +type Action int +type metadataStateType int + +const ( + IssueMetadata metadataStateType = iota + PRMetadata +) + +type IssueMetadataState struct { + Type metadataStateType + + Body string + Title string + + Metadata []string + Reviewers []string + Assignees []string + Labels []string + Projects []string + Milestones []string + + MetadataResult *api.RepoMetadataResult +} + +func (tb *IssueMetadataState) HasMetadata() bool { + return len(tb.Reviewers) > 0 || + len(tb.Assignees) > 0 || + len(tb.Labels) > 0 || + len(tb.Projects) > 0 || + len(tb.Milestones) > 0 +} + +const ( + SubmitAction Action = iota + PreviewAction + CancelAction + MetadataAction + + noMilestone = "(none)" +) + +func ConfirmSubmission(allowPreview bool, allowMetadata bool) (Action, error) { + const ( + submitLabel = "Submit" + previewLabel = "Continue in browser" + metadataLabel = "Add metadata" + cancelLabel = "Cancel" + ) + + options := []string{submitLabel} + if allowPreview { + options = append(options, previewLabel) + } + if allowMetadata { + options = append(options, metadataLabel) + } + options = append(options, cancelLabel) + + confirmAnswers := struct { + Confirmation int + }{} + confirmQs := []*survey.Question{ + { + Name: "confirmation", + Prompt: &survey.Select{ + Message: "What's next?", + Options: options, + }, + }, + } + + err := prompt.SurveyAsk(confirmQs, &confirmAnswers) + if err != nil { + return -1, fmt.Errorf("could not prompt: %w", err) + } + + switch options[confirmAnswers.Confirmation] { + case submitLabel: + return SubmitAction, nil + case previewLabel: + return PreviewAction, nil + case metadataLabel: + return MetadataAction, nil + case cancelLabel: + return CancelAction, nil + default: + return -1, fmt.Errorf("invalid index: %d", confirmAnswers.Confirmation) + } +} + +func TemplateSurvey(templateFiles []string, legacyTemplate string, state IssueMetadataState) (templateContent string, err error) { + if len(templateFiles) == 0 && legacyTemplate == "" { + return + } + + if len(templateFiles) > 0 { + templateContent, err = selectTemplate(templateFiles, legacyTemplate, state.Type) + } else { + templateContent = string(githubtemplate.ExtractContents(legacyTemplate)) + } + + return +} + +func selectTemplate(nonLegacyTemplatePaths []string, legacyTemplatePath string, metadataType metadataStateType) (string, error) { + templateResponse := struct { + Index int + }{} + templateNames := make([]string, 0, len(nonLegacyTemplatePaths)) + for _, p := range nonLegacyTemplatePaths { + templateNames = append(templateNames, githubtemplate.ExtractName(p)) + } + if metadataType == IssueMetadata { + templateNames = append(templateNames, "Open a blank issue") + } else if metadataType == PRMetadata { + templateNames = append(templateNames, "Open a blank pull request") + } + + selectQs := []*survey.Question{ + { + Name: "index", + Prompt: &survey.Select{ + Message: "Choose a template", + Options: templateNames, + }, + }, + } + if err := prompt.SurveyAsk(selectQs, &templateResponse); err != nil { + return "", fmt.Errorf("could not prompt: %w", err) + } + + if templateResponse.Index == len(nonLegacyTemplatePaths) { // the user has selected the blank template + if legacyTemplatePath != "" { + templateContents := githubtemplate.ExtractContents(legacyTemplatePath) + return string(templateContents), nil + } else { + return "", nil + } + } + templateContents := githubtemplate.ExtractContents(nonLegacyTemplatePaths[templateResponse.Index]) + return string(templateContents), nil +} + +func BodySurvey(state *IssueMetadataState, templateContent, editorCommand string) error { + if templateContent != "" { + if state.Body != "" { + // prevent excessive newlines between default body and template + state.Body = strings.TrimRight(state.Body, "\n") + state.Body += "\n\n" + } + state.Body += templateContent + } + + p := &surveyext.GhEditor{ + BlankAllowed: true, + EditorCommand: editorCommand, + Editor: &survey.Editor{ + Message: "Body", + FileName: "*.md", + Default: state.Body, + HideDefault: true, + AppendDefault: true, + }, + } + + body := "" + + err := prompt.SurveyAskOne(p, &body) + if err != nil { + return err + } + + state.Body = body + + return nil +} + +func TitleSurvey(state *IssueMetadataState) error { + p := &survey.Input{ + Message: "Title", + Default: state.Title, + } + + title := "" + + err := prompt.SurveyAskOne(p, &title) + if err != nil { + return err + } + + state.Title = title + + return nil +} + +func MetadataSurvey(io *iostreams.IOStreams, client *api.Client, baseRepo ghrepo.Interface, state *IssueMetadataState) error { + isChosen := func(m string) bool { + for _, c := range state.Metadata { + if m == c { + return true + } + } + return false + } + + allowReviewers := state.Type == PRMetadata + + extraFieldsOptions := []string{} + if allowReviewers { + extraFieldsOptions = append(extraFieldsOptions, "Reviewers") + } + extraFieldsOptions = append(extraFieldsOptions, "Assignees", "Labels", "Projects", "Milestone") + + err := prompt.SurveyAsk([]*survey.Question{ + { + Name: "metadata", + Prompt: &survey.MultiSelect{ + Message: "What would you like to add?", + Options: extraFieldsOptions, + }, + }, + }, state) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + metadataInput := api.RepoMetadataInput{ + Reviewers: isChosen("Reviewers"), + Assignees: isChosen("Assignees"), + Labels: isChosen("Labels"), + Projects: isChosen("Projects"), + Milestones: isChosen("Milestone"), + } + s := utils.Spinner(io.ErrOut) + utils.StartSpinner(s) + state.MetadataResult, err = api.RepoMetadata(client, baseRepo, metadataInput) + utils.StopSpinner(s) + if err != nil { + return fmt.Errorf("error fetching metadata options: %w", err) + } + + var users []string + for _, u := range state.MetadataResult.AssignableUsers { + users = append(users, u.Login) + } + var teams []string + for _, t := range state.MetadataResult.Teams { + teams = append(teams, fmt.Sprintf("%s/%s", baseRepo.RepoOwner(), t.Slug)) + } + var labels []string + for _, l := range state.MetadataResult.Labels { + labels = append(labels, l.Name) + } + var projects []string + for _, l := range state.MetadataResult.Projects { + projects = append(projects, l.Name) + } + milestones := []string{noMilestone} + for _, m := range state.MetadataResult.Milestones { + milestones = append(milestones, m.Title) + } + + type metadataValues struct { + Reviewers []string + Assignees []string + Labels []string + Projects []string + Milestone string + } + var mqs []*survey.Question + if isChosen("Reviewers") { + if len(users) > 0 || len(teams) > 0 { + mqs = append(mqs, &survey.Question{ + Name: "reviewers", + Prompt: &survey.MultiSelect{ + Message: "Reviewers", + Options: append(users, teams...), + Default: state.Reviewers, + }, + }) + } else { + fmt.Fprintln(io.ErrOut, "warning: no available reviewers") + } + } + if isChosen("Assignees") { + if len(users) > 0 { + mqs = append(mqs, &survey.Question{ + Name: "assignees", + Prompt: &survey.MultiSelect{ + Message: "Assignees", + Options: users, + Default: state.Assignees, + }, + }) + } else { + fmt.Fprintln(io.ErrOut, "warning: no assignable users") + } + } + if isChosen("Labels") { + if len(labels) > 0 { + mqs = append(mqs, &survey.Question{ + Name: "labels", + Prompt: &survey.MultiSelect{ + Message: "Labels", + Options: labels, + Default: state.Labels, + }, + }) + } else { + fmt.Fprintln(io.ErrOut, "warning: no labels in the repository") + } + } + if isChosen("Projects") { + if len(projects) > 0 { + mqs = append(mqs, &survey.Question{ + Name: "projects", + Prompt: &survey.MultiSelect{ + Message: "Projects", + Options: projects, + Default: state.Projects, + }, + }) + } else { + fmt.Fprintln(io.ErrOut, "warning: no projects to choose from") + } + } + if isChosen("Milestone") { + if len(milestones) > 1 { + var milestoneDefault string + if len(state.Milestones) > 0 { + milestoneDefault = state.Milestones[0] + } + mqs = append(mqs, &survey.Question{ + Name: "milestone", + Prompt: &survey.Select{ + Message: "Milestone", + Options: milestones, + Default: milestoneDefault, + }, + }) + } else { + fmt.Fprintln(io.ErrOut, "warning: no milestones in the repository") + } + } + values := metadataValues{} + err = prompt.SurveyAsk(mqs, &values, survey.WithKeepFilter(true)) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + state.Reviewers = values.Reviewers + state.Assignees = values.Assignees + state.Labels = values.Labels + state.Projects = values.Projects + if values.Milestone != "" && values.Milestone != noMilestone { + state.Milestones = []string{values.Milestone} + } + + return nil +} + +func FindTemplates(dir, path string) ([]string, string) { + if dir == "" { + rootDir, err := git.ToplevelDir() + if err != nil { + return []string{}, "" + } + dir = rootDir + } + + templateFiles := githubtemplate.FindNonLegacy(dir, path) + legacyTemplate := githubtemplate.FindLegacy(dir, path) + + // TODO stop using string pointer + + lt := "" + + if legacyTemplate != nil { + lt = *legacyTemplate + } + + return templateFiles, lt +} diff --git a/pkg/cmd/pr/shared/title_body_survey.go b/pkg/cmd/pr/shared/title_body_survey.go deleted file mode 100644 index 83a50e0ff..000000000 --- a/pkg/cmd/pr/shared/title_body_survey.go +++ /dev/null @@ -1,396 +0,0 @@ -package shared - -import ( - "fmt" - "strings" - - "github.com/AlecAivazis/survey/v2" - "github.com/cli/cli/api" - "github.com/cli/cli/internal/ghrepo" - "github.com/cli/cli/pkg/githubtemplate" - "github.com/cli/cli/pkg/iostreams" - "github.com/cli/cli/pkg/prompt" - "github.com/cli/cli/pkg/surveyext" - "github.com/cli/cli/utils" -) - -type Defaults struct { - Title string - Body string -} - -type Action int -type metadataStateType int - -const ( - IssueMetadata metadataStateType = iota - PRMetadata -) - -type IssueMetadataState struct { - Type metadataStateType - - Body string - Title string - Action Action - - Metadata []string - Reviewers []string - Assignees []string - Labels []string - Projects []string - Milestones []string - - MetadataResult *api.RepoMetadataResult -} - -func (tb *IssueMetadataState) HasMetadata() bool { - return len(tb.Reviewers) > 0 || - len(tb.Assignees) > 0 || - len(tb.Labels) > 0 || - len(tb.Projects) > 0 || - len(tb.Milestones) > 0 -} - -const ( - SubmitAction Action = iota - PreviewAction - CancelAction - MetadataAction - - noMilestone = "(none)" -) - -func confirmSubmission(allowPreview bool, allowMetadata bool) (Action, error) { - const ( - submitLabel = "Submit" - previewLabel = "Continue in browser" - metadataLabel = "Add metadata" - cancelLabel = "Cancel" - ) - - options := []string{submitLabel} - if allowPreview { - options = append(options, previewLabel) - } - if allowMetadata { - options = append(options, metadataLabel) - } - options = append(options, cancelLabel) - - confirmAnswers := struct { - Confirmation int - }{} - confirmQs := []*survey.Question{ - { - Name: "confirmation", - Prompt: &survey.Select{ - Message: "What's next?", - Options: options, - }, - }, - } - - err := prompt.SurveyAsk(confirmQs, &confirmAnswers) - if err != nil { - return -1, fmt.Errorf("could not prompt: %w", err) - } - - switch options[confirmAnswers.Confirmation] { - case submitLabel: - return SubmitAction, nil - case previewLabel: - return PreviewAction, nil - case metadataLabel: - return MetadataAction, nil - case cancelLabel: - return CancelAction, nil - default: - return -1, fmt.Errorf("invalid index: %d", confirmAnswers.Confirmation) - } -} - -func selectTemplate(nonLegacyTemplatePaths []string, legacyTemplatePath *string, metadataType metadataStateType) (string, error) { - templateResponse := struct { - Index int - }{} - templateNames := make([]string, 0, len(nonLegacyTemplatePaths)) - for _, p := range nonLegacyTemplatePaths { - templateNames = append(templateNames, githubtemplate.ExtractName(p)) - } - if metadataType == IssueMetadata { - templateNames = append(templateNames, "Open a blank issue") - } else if metadataType == PRMetadata { - templateNames = append(templateNames, "Open a blank pull request") - } - - selectQs := []*survey.Question{ - { - Name: "index", - Prompt: &survey.Select{ - Message: "Choose a template", - Options: templateNames, - }, - }, - } - if err := prompt.SurveyAsk(selectQs, &templateResponse); err != nil { - return "", fmt.Errorf("could not prompt: %w", err) - } - - if templateResponse.Index == len(nonLegacyTemplatePaths) { // the user has selected the blank template - if legacyTemplatePath != nil { - templateContents := githubtemplate.ExtractContents(*legacyTemplatePath) - return string(templateContents), nil - } else { - return "", nil - } - } - templateContents := githubtemplate.ExtractContents(nonLegacyTemplatePaths[templateResponse.Index]) - return string(templateContents), nil -} - -// FIXME: this command has too many parameters and responsibilities -func TitleBodySurvey(io *iostreams.IOStreams, editorCommand string, issueState *IssueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs Defaults, nonLegacyTemplatePaths []string, legacyTemplatePath *string, allowReviewers, allowMetadata bool) error { - issueState.Title = defs.Title - templateContents := "" - - if providedBody == "" { - issueState.Body = defs.Body - - if len(nonLegacyTemplatePaths) > 0 { - var err error - templateContents, err = selectTemplate(nonLegacyTemplatePaths, legacyTemplatePath, issueState.Type) - if err != nil { - return err - } - - } else if legacyTemplatePath != nil { - templateContents = string(githubtemplate.ExtractContents(*legacyTemplatePath)) - } - - if templateContents != "" { - if issueState.Body != "" { - // prevent excessive newlines between default body and template - issueState.Body = strings.TrimRight(issueState.Body, "\n") - issueState.Body += "\n\n" - } - issueState.Body += templateContents - } - } - - titleQuestion := &survey.Question{ - Name: "title", - Prompt: &survey.Input{ - Message: "Title", - Default: issueState.Title, - }, - } - bodyQuestion := &survey.Question{ - Name: "body", - Prompt: &surveyext.GhEditor{ - BlankAllowed: true, - EditorCommand: editorCommand, - Editor: &survey.Editor{ - Message: "Body", - FileName: "*.md", - Default: issueState.Body, - HideDefault: true, - AppendDefault: true, - }, - }, - } - - var qs []*survey.Question - if providedTitle == "" { - qs = append(qs, titleQuestion) - } - if providedBody == "" { - qs = append(qs, bodyQuestion) - } - - err := prompt.SurveyAsk(qs, issueState) - if err != nil { - return fmt.Errorf("could not prompt: %w", err) - } - - if issueState.Body == "" { - issueState.Body = templateContents - } - - allowPreview := !issueState.HasMetadata() - confirmA, err := confirmSubmission(allowPreview, allowMetadata) - if err != nil { - return fmt.Errorf("unable to confirm: %w", err) - } - - if confirmA == MetadataAction { - isChosen := func(m string) bool { - for _, c := range issueState.Metadata { - if m == c { - return true - } - } - return false - } - - extraFieldsOptions := []string{} - if allowReviewers { - extraFieldsOptions = append(extraFieldsOptions, "Reviewers") - } - extraFieldsOptions = append(extraFieldsOptions, "Assignees", "Labels", "Projects", "Milestone") - - err = prompt.SurveyAsk([]*survey.Question{ - { - Name: "metadata", - Prompt: &survey.MultiSelect{ - Message: "What would you like to add?", - Options: extraFieldsOptions, - }, - }, - }, issueState) - if err != nil { - return fmt.Errorf("could not prompt: %w", err) - } - - metadataInput := api.RepoMetadataInput{ - Reviewers: isChosen("Reviewers"), - Assignees: isChosen("Assignees"), - Labels: isChosen("Labels"), - Projects: isChosen("Projects"), - Milestones: isChosen("Milestone"), - } - s := utils.Spinner(io.ErrOut) - utils.StartSpinner(s) - issueState.MetadataResult, err = api.RepoMetadata(apiClient, repo, metadataInput) - utils.StopSpinner(s) - if err != nil { - return fmt.Errorf("error fetching metadata options: %w", err) - } - - var users []string - for _, u := range issueState.MetadataResult.AssignableUsers { - users = append(users, u.Login) - } - var teams []string - for _, t := range issueState.MetadataResult.Teams { - teams = append(teams, fmt.Sprintf("%s/%s", repo.RepoOwner(), t.Slug)) - } - var labels []string - for _, l := range issueState.MetadataResult.Labels { - labels = append(labels, l.Name) - } - var projects []string - for _, l := range issueState.MetadataResult.Projects { - projects = append(projects, l.Name) - } - milestones := []string{noMilestone} - for _, m := range issueState.MetadataResult.Milestones { - milestones = append(milestones, m.Title) - } - - type metadataValues struct { - Reviewers []string - Assignees []string - Labels []string - Projects []string - Milestone string - } - var mqs []*survey.Question - if isChosen("Reviewers") { - if len(users) > 0 || len(teams) > 0 { - mqs = append(mqs, &survey.Question{ - Name: "reviewers", - Prompt: &survey.MultiSelect{ - Message: "Reviewers", - Options: append(users, teams...), - Default: issueState.Reviewers, - }, - }) - } else { - fmt.Fprintln(io.ErrOut, "warning: no available reviewers") - } - } - if isChosen("Assignees") { - if len(users) > 0 { - mqs = append(mqs, &survey.Question{ - Name: "assignees", - Prompt: &survey.MultiSelect{ - Message: "Assignees", - Options: users, - Default: issueState.Assignees, - }, - }) - } else { - fmt.Fprintln(io.ErrOut, "warning: no assignable users") - } - } - if isChosen("Labels") { - if len(labels) > 0 { - mqs = append(mqs, &survey.Question{ - Name: "labels", - Prompt: &survey.MultiSelect{ - Message: "Labels", - Options: labels, - Default: issueState.Labels, - }, - }) - } else { - fmt.Fprintln(io.ErrOut, "warning: no labels in the repository") - } - } - if isChosen("Projects") { - if len(projects) > 0 { - mqs = append(mqs, &survey.Question{ - Name: "projects", - Prompt: &survey.MultiSelect{ - Message: "Projects", - Options: projects, - Default: issueState.Projects, - }, - }) - } else { - fmt.Fprintln(io.ErrOut, "warning: no projects to choose from") - } - } - if isChosen("Milestone") { - if len(milestones) > 1 { - var milestoneDefault string - if len(issueState.Milestones) > 0 { - milestoneDefault = issueState.Milestones[0] - } - mqs = append(mqs, &survey.Question{ - Name: "milestone", - Prompt: &survey.Select{ - Message: "Milestone", - Options: milestones, - Default: milestoneDefault, - }, - }) - } else { - fmt.Fprintln(io.ErrOut, "warning: no milestones in the repository") - } - } - values := metadataValues{} - err = prompt.SurveyAsk(mqs, &values, survey.WithKeepFilter(true)) - if err != nil { - return fmt.Errorf("could not prompt: %w", err) - } - issueState.Reviewers = values.Reviewers - issueState.Assignees = values.Assignees - issueState.Labels = values.Labels - issueState.Projects = values.Projects - if values.Milestone != "" && values.Milestone != noMilestone { - issueState.Milestones = []string{values.Milestone} - } - - allowPreview = !issueState.HasMetadata() - allowMetadata = false - confirmA, err = confirmSubmission(allowPreview, allowMetadata) - if err != nil { - return fmt.Errorf("unable to confirm: %w", err) - } - } - - issueState.Action = confirmA - return nil -} diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index cd54ae210..64d0c0d4a 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -54,6 +54,7 @@ mainLoop: // FindLegacy returns the file path of the default(legacy) template func FindLegacy(rootDir string, name string) *string { + // TODO why does this return a pointer to string?? namePattern := regexp.MustCompile(fmt.Sprintf(`(?i)^%s(\.|$)`, strings.ReplaceAll(name, "_", "[_-]"))) // https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository From 1c280d434109229ab5d804032f35880e9cae636c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 10 Nov 2020 16:18:36 -0800 Subject: [PATCH 3/7] stop using string pointer --- pkg/cmd/pr/shared/survey.go | 10 +--------- pkg/githubtemplate/github_template.go | 8 +++----- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index a1a3f65fd..da995925e 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -392,13 +392,5 @@ func FindTemplates(dir, path string) ([]string, string) { templateFiles := githubtemplate.FindNonLegacy(dir, path) legacyTemplate := githubtemplate.FindLegacy(dir, path) - // TODO stop using string pointer - - lt := "" - - if legacyTemplate != nil { - lt = *legacyTemplate - } - - return templateFiles, lt + return templateFiles, legacyTemplate } diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index 64d0c0d4a..4ae0ff4b1 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -53,8 +53,7 @@ mainLoop: } // FindLegacy returns the file path of the default(legacy) template -func FindLegacy(rootDir string, name string) *string { - // TODO why does this return a pointer to string?? +func FindLegacy(rootDir string, name string) string { namePattern := regexp.MustCompile(fmt.Sprintf(`(?i)^%s(\.|$)`, strings.ReplaceAll(name, "_", "[_-]"))) // https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository @@ -72,12 +71,11 @@ func FindLegacy(rootDir string, name string) *string { // detect a single template file for _, file := range files { if namePattern.MatchString(file.Name()) && !file.IsDir() { - result := path.Join(dir, file.Name()) - return &result + return path.Join(dir, file.Name()) } } } - return nil + return "" } // ExtractName returns the name of the template from YAML front-matter From 0ed78793299fcdac782bc80c5f7c13493c75c1c8 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 10 Nov 2020 16:25:18 -0800 Subject: [PATCH 4/7] stop using Defaults struct --- pkg/cmd/pr/create/create.go | 33 +++++++++++++++------------------ pkg/cmd/pr/shared/survey.go | 5 ----- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 32bd5807d..eac02a31a 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -159,19 +159,12 @@ func createRun(opts *CreateOptions) (err error) { return err } - defs, defaultsErr := computeDefaults(*ctx) - if defaultsErr != nil && (opts.Autofill || opts.WebMode || !opts.Interactive) { - return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) - } - var milestoneTitles []string if opts.Milestone != "" { milestoneTitles = []string{opts.Milestone} } state := shared.IssueMetadataState{ - Title: defs.Title, - Body: defs.Body, Type: shared.PRMetadata, Reviewers: opts.Reviewers, Assignees: opts.Assignees, @@ -180,6 +173,11 @@ func createRun(opts *CreateOptions) (err error) { Milestones: milestoneTitles, } + defaultsErr := computeDefaults(*ctx, &state) + if defaultsErr != nil && (opts.Autofill || opts.WebMode || !opts.Interactive) { + return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) + } + if opts.TitleProvided { state.Title = opts.Title } @@ -297,34 +295,33 @@ func createRun(opts *CreateOptions) (err error) { return errors.New("expected to cancel, preview, or submit") } -func computeDefaults(createCtx CreateContext) (shared.Defaults, error) { - baseRef := createCtx.BaseTrackingBranch - headRef := createCtx.HeadBranch - out := shared.Defaults{} +func computeDefaults(ctx CreateContext, state *shared.IssueMetadataState) error { + baseRef := ctx.BaseTrackingBranch + headRef := ctx.HeadBranch commits, err := git.Commits(baseRef, headRef) if err != nil { - return out, err + return err } if len(commits) == 1 { - out.Title = commits[0].Title + state.Title = commits[0].Title body, err := git.CommitBody(commits[0].Sha) if err != nil { - return out, err + return err } - out.Body = body + state.Body = body } else { - out.Title = utils.Humanize(headRef) + state.Title = utils.Humanize(headRef) var body strings.Builder for i := len(commits) - 1; i >= 0; i-- { fmt.Fprintf(&body, "- %s\n", commits[i].Title) } - out.Body = body.String() + state.Body = body.String() } - return out, nil + return nil } func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.TrackingRef { diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index da995925e..d73846113 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -15,11 +15,6 @@ import ( "github.com/cli/cli/utils" ) -type Defaults struct { - Title string - Body string -} - type Action int type metadataStateType int From f5277e452e3e29a51c866e8bcf96e2b33b7b5b0a Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 10 Nov 2020 16:27:06 -0800 Subject: [PATCH 5/7] get everything working --- pkg/cmd/issue/create/create.go | 36 ++++--- pkg/cmd/issue/create/create_test.go | 11 +- pkg/cmd/pr/create/create.go | 115 ++++++++++----------- pkg/cmd/pr/create/create_test.go | 86 ++++++--------- pkg/cmd/pr/shared/survey.go | 12 +-- pkg/githubtemplate/github_template_test.go | 6 +- pkg/prompt/stubber.go | 2 +- 7 files changed, 118 insertions(+), 150 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 2d5e86898..28c8f0148 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -154,25 +154,29 @@ func createRun(opts *CreateOptions) error { return err } - err = prShared.TitleSurvey(&tb) - if err != nil { - return err - } - - templateContent := "" - - templateContent, err = prShared.TemplateSurvey(templateFiles, legacyTemplate, tb) - if err != nil { - return err - } - - err = prShared.BodySurvey(&tb, templateContent, editorCommand) - if err != nil { - return err + if tb.Title == "" { + err = prShared.TitleSurvey(&tb) + if err != nil { + return err + } } if tb.Body == "" { - tb.Body = templateContent + templateContent := "" + + templateContent, err = prShared.TemplateSurvey(templateFiles, legacyTemplate, tb) + if err != nil { + return err + } + + err = prShared.BodySurvey(&tb, templateContent, editorCommand) + if err != nil { + return err + } + + if tb.Body == "" { + tb.Body = templateContent + } } action, err := prShared.ConfirmSubmission(!tb.HasMetadata(), repo.ViewerCanTriage()) diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 2216f5b1d..37dba3407 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -144,18 +144,17 @@ func TestIssueCreate_nonLegacyTemplate(t *testing.T) { as, teardown := prompt.InitAskStubber() defer teardown() + + // tmeplate as.Stub([]*prompt.QuestionStub{ { Name: "index", Value: 1, }, }) - as.Stub([]*prompt.QuestionStub{ - { - Name: "body", - Default: true, - }, - }) + // body + as.StubOneDefault() + // confirm as.Stub([]*prompt.QuestionStub{ { Name: "confirmation", diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index eac02a31a..8dd21ca33 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -32,8 +32,6 @@ type CreateOptions struct { Remotes func() (context.Remotes, error) Branch func() (string, error) - Interactive bool - TitleProvided bool BodyProvided bool @@ -60,7 +58,7 @@ type CreateContext struct { // This struct stores contextual data about the creation process and is for building up enough // data to create a pull request RepoContext *context.ResolvedRemotes - BaseRepo *api.Repository + BaseRepo ghrepo.Interface HeadRepo ghrepo.Interface BaseTrackingBranch string BaseBranch string @@ -68,6 +66,7 @@ type CreateContext struct { HeadBranchLabel string HeadRemote *context.Remote IsPushEnabled bool + Client *api.Client } func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command { @@ -104,18 +103,10 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.BodyProvided = cmd.Flags().Changed("body") opts.RepoOverride, _ = cmd.Flags().GetString("repo") - opts.Interactive = !(opts.TitleProvided && opts.BodyProvided) - - // TODO check on edge cases around title/body provision - if !opts.IO.CanPrompt() && !opts.WebMode && !opts.TitleProvided && !opts.Autofill { return &cmdutil.FlagError{Err: errors.New("--title or --fill required when not running interactively")} } - if !opts.IO.CanPrompt() { - opts.Interactive = false - } - if opts.IsDraft && opts.WebMode { return errors.New("the --draft flag is not supported with --web") } @@ -148,17 +139,13 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) (err error) { - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - client := api.NewClientFromHTTP(httpClient) - ctx, err := NewCreateContext(opts) if err != nil { return err } + client := ctx.Client + var milestoneTitles []string if opts.Milestone != "" { milestoneTitles = []string{opts.Milestone} @@ -173,9 +160,24 @@ func createRun(opts *CreateOptions) (err error) { Milestones: milestoneTitles, } - defaultsErr := computeDefaults(*ctx, &state) - if defaultsErr != nil && (opts.Autofill || opts.WebMode || !opts.Interactive) { - return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) + var defaultsErr error + if opts.Autofill || !opts.TitleProvided || !opts.BodyProvided { + defaultsErr = computeDefaults(*ctx, &state) + if defaultsErr != nil { + return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) + } + } + + if opts.WebMode { + if !opts.Autofill { + state.Title = opts.Title + state.Body = opts.Body + } + err := handlePush(*opts, *ctx) + if err != nil { + return err + } + return previewPR(*opts, *ctx, state) } if opts.TitleProvided { @@ -186,18 +188,6 @@ func createRun(opts *CreateOptions) (err error) { state.Body = opts.Body } - if opts.WebMode { - err := handlePush(*opts, *ctx) - if err != nil { - return err - } - return previewPR(*opts, *ctx, state) - } - - if opts.Autofill || !opts.Interactive { - return submitPR(*opts, *ctx, state) - } - existingPR, err := api.PullRequestForBranch( client, ctx.BaseRepo, ctx.BaseBranch, ctx.HeadBranchLabel, []string{"OPEN"}) var notFound *api.NotFoundError @@ -216,13 +206,23 @@ func createRun(opts *CreateOptions) (err error) { cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.ErrOut, message, - cs.Cyan(ctx.HeadBranchLabel), - cs.Cyan(ctx.BaseBranch), - ghrepo.FullName(ctx.BaseRepo)) - if (state.Title == "" || state.Body == "") && defaultsErr != nil { - fmt.Fprintf(opts.IO.ErrOut, - "%s warning: could not compute title or body defaults: %s\n", cs.Yellow("!"), defaultsErr) + if opts.IO.CanPrompt() { + fmt.Fprintf(opts.IO.ErrOut, message, + cs.Cyan(ctx.HeadBranchLabel), + cs.Cyan(ctx.BaseBranch), + ghrepo.FullName(ctx.BaseRepo)) + if (state.Title == "" || state.Body == "") && defaultsErr != nil { + fmt.Fprintf(opts.IO.ErrOut, + "%s warning: could not compute title or body defaults: %s\n", cs.Yellow("!"), defaultsErr) + } + } + + if opts.Autofill || (opts.TitleProvided && opts.BodyProvided) { + err = handlePush(*opts, *ctx) + if err != nil { + return err + } + return submitPR(*opts, *ctx, state) } if !opts.TitleProvided { @@ -256,7 +256,7 @@ func createRun(opts *CreateOptions) (err error) { } } - allowMetadata := ctx.BaseRepo.ViewerCanTriage() + allowMetadata := ctx.BaseRepo.(*api.Repository).ViewerCanTriage() action, err := shared.ConfirmSubmission(!state.HasMetadata(), allowMetadata) if err != nil { return fmt.Errorf("unable to confirm: %w", err) @@ -523,35 +523,32 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { HeadRemote: headRemote, IsPushEnabled: isPushEnabled, RepoContext: repoContext, + Client: client, }, nil } -func submitPR(opts CreateOptions, createCtx CreateContext, state shared.IssueMetadataState) error { - httpClient, err := opts.HttpClient() - if err != nil { - return nil - } - client := api.NewClientFromHTTP(httpClient) +func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataState) error { + client := ctx.Client params := map[string]interface{}{ "title": state.Title, "body": state.Body, "draft": opts.IsDraft, - "baseRefName": createCtx.BaseBranch, - "headRefName": createCtx.HeadBranchLabel, + "baseRefName": ctx.BaseBranch, + "headRefName": ctx.HeadBranchLabel, } if params["title"] == "" { return errors.New("pull request title must not be blank") } - err = shared.AddMetadataToIssueParams(client, createCtx.BaseRepo, params, &state) + err := shared.AddMetadataToIssueParams(client, ctx.BaseRepo, params, &state) if err != nil { return err } - pr, err := api.CreatePullRequest(client, createCtx.BaseRepo, params) + pr, err := api.CreatePullRequest(client, ctx.BaseRepo.(*api.Repository), params) if pr != nil { fmt.Fprintln(opts.IO.Out, pr.URL) } @@ -564,8 +561,8 @@ func submitPR(opts CreateOptions, createCtx CreateContext, state shared.IssueMet return nil } -func previewPR(opts CreateOptions, createCtx CreateContext, state shared.IssueMetadataState) error { - openURL, err := generateCompareURL(createCtx, state) +func previewPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataState) error { + openURL, err := generateCompareURL(ctx, state) if err != nil { return err } @@ -581,13 +578,9 @@ func handlePush(opts CreateOptions, ctx CreateContext) error { didForkRepo := false headRepo := ctx.HeadRepo headRemote := ctx.HeadRemote + client := ctx.Client - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - client := api.NewClientFromHTTP(httpClient) - + var err error // if a head repository could not be determined so far, automatically create // one by forking the base repository if headRepo == nil && ctx.IsPushEnabled { @@ -664,11 +657,11 @@ func handlePush(opts CreateOptions, ctx CreateContext) error { return nil } -func generateCompareURL(createCtx CreateContext, state shared.IssueMetadataState) (string, error) { +func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (string, error) { u := ghrepo.GenerateRepoURL( - createCtx.BaseRepo, + ctx.BaseRepo, "compare/%s...%s?expand=1", - url.QueryEscape(createCtx.BaseBranch), url.QueryEscape(createCtx.HeadBranch)) + url.QueryEscape(ctx.BaseBranch), url.QueryEscape(ctx.HeadBranch)) url, err := shared.WithPrAndIssueQueryParams(u, state) if err != nil { return "", err diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 456da84a9..2e7192912 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -15,6 +15,7 @@ import ( "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + prShared "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -270,12 +271,11 @@ func TestPRCreate_createFork(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - cs.Stub("") // git config --get-regexp (determineTrackingBranch) - cs.Stub("") // git show-ref --verify (determineTrackingBranch) - cs.Stub("") // git status - cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log - cs.Stub("") // git remote add - cs.Stub("") // git push + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) + cs.Stub("") // git status + cs.Stub("") // git remote add + cs.Stub("") // git push ask, cleanupAsk := prompt.InitAskStubber() defer cleanupAsk() @@ -284,8 +284,8 @@ func TestPRCreate_createFork(t *testing.T) { output, err := runCommand(http, nil, "feature", true, `-t title -b body`) require.NoError(t, err) - assert.Equal(t, []string{"git", "remote", "add", "-f", "fork", "https://github.com/monalisa/REPO.git"}, cs.Calls[4].Args) - assert.Equal(t, []string{"git", "push", "--set-upstream", "fork", "HEAD:feature"}, cs.Calls[5].Args) + assert.Equal(t, []string{"git", "remote", "add", "-f", "fork", "https://github.com/monalisa/REPO.git"}, cs.Calls[3].Args) + assert.Equal(t, []string{"git", "push", "--set-upstream", "fork", "HEAD:feature"}, cs.Calls[4].Args) assert.Equal(t, "https://github.com/OWNER/REPO/pull/12\n", output.String()) } @@ -340,9 +340,6 @@ func TestPRCreate_pushedToNonBaseRepo(t *testing.T) { deadb00f refs/remotes/upstream/feature deadbeef refs/remotes/origin/feature `)) // determineTrackingBranch - cs.Register("git .+ log", 1, "", func(args []string) { - assert.Equal(t, "upstream/master...feature", args[len(args)-1]) - }) _, cleanupAsk := prompt.InitAskStubber() defer cleanupAsk() @@ -389,9 +386,6 @@ func TestPRCreate_pushedToDifferentBranchName(t *testing.T) { deadbeef HEAD deadbeef refs/remotes/origin/my-feat2 `)) // determineTrackingBranch - cs.Register("git .+ log", 1, "", func(args []string) { - assert.Equal(t, "origin/master...feature", args[len(args)-1]) - }) _, cleanupAsk := prompt.InitAskStubber() defer cleanupAsk() @@ -438,19 +432,14 @@ func TestPRCreate_nonLegacyTemplate(t *testing.T) { Name: "index", Value: 0, }, - }) - as.Stub([]*prompt.QuestionStub{ - { - Name: "body", - Default: true, - }, - }) + }) // template + as.StubOneDefault() // body as.Stub([]*prompt.QuestionStub{ { Name: "confirmation", Value: 0, }, - }) + }) // confirm output, err := runCommandWithRootDirOverridden(http, nil, "feature", true, `-t "my title" -H feature`, "./fixtures/repoWithNonLegacyPRTemplates") require.NoError(t, err) @@ -586,12 +575,11 @@ func TestPRCreate_alreadyExists(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - cs.Stub("") // git config --get-regexp (determineTrackingBranch) - cs.Stub("") // git show-ref --verify (determineTrackingBranch) - cs.Stub("") // git status - cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) + cs.Stub("") // git status - _, err := runCommand(http, nil, "feature", true, `-H feature`) + _, err := runCommand(http, nil, "feature", true, `-ttitle -bbody -H feature`) if err == nil { t.Fatal("error expected, got nil") } @@ -732,50 +720,42 @@ deadb00f refs/remotes/origin/feature`) // git show-ref --verify (ShowRefs) } func Test_generateCompareURL(t *testing.T) { - type args struct { - r ghrepo.Interface - base string - head string - title string - body string - assignees []string - labels []string - projects []string - milestones []string - } tests := []struct { name string - args args + ctx CreateContext + state prShared.IssueMetadataState want string wantErr bool }{ { name: "basic", - args: args{ - r: ghrepo.New("OWNER", "REPO"), - base: "main", - head: "feature", + ctx: CreateContext{ + BaseRepo: ghrepo.New("OWNER", "REPO"), + BaseBranch: "main", + HeadBranch: "feature", }, want: "https://github.com/OWNER/REPO/compare/main...feature?expand=1", wantErr: false, }, { name: "with labels", - args: args{ - r: ghrepo.New("OWNER", "REPO"), - base: "a", - head: "b", - labels: []string{"one", "two three"}, + ctx: CreateContext{ + BaseRepo: ghrepo.New("OWNER", "REPO"), + BaseBranch: "a", + HeadBranch: "b", + }, + state: prShared.IssueMetadataState{ + Labels: []string{"one", "two three"}, }, want: "https://github.com/OWNER/REPO/compare/a...b?expand=1&labels=one%2Ctwo+three", wantErr: false, }, { name: "complex branch names", - args: args{ - r: ghrepo.New("OWNER", "REPO"), - base: "main/trunk", - head: "owner:feature", + ctx: CreateContext{ + BaseRepo: ghrepo.New("OWNER", "REPO"), + BaseBranch: "main/trunk", + HeadBranch: "owner:feature", }, want: "https://github.com/OWNER/REPO/compare/main%2Ftrunk...owner%3Afeature?expand=1", wantErr: false, @@ -783,7 +763,7 @@ func Test_generateCompareURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := generateCompareURL(tt.args.r, tt.args.base, tt.args.head, tt.args.title, tt.args.body, tt.args.assignees, tt.args.labels, tt.args.projects, tt.args.milestones) + got, err := generateCompareURL(tt.ctx, tt.state) if (err != nil) != tt.wantErr { t.Errorf("generateCompareURL() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index d73846113..37d493d82 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -180,15 +180,11 @@ func BodySurvey(state *IssueMetadataState, templateContent, editorCommand string }, } - body := "" - - err := prompt.SurveyAskOne(p, &body) + err := prompt.SurveyAskOne(p, state) if err != nil { return err } - state.Body = body - return nil } @@ -198,15 +194,11 @@ func TitleSurvey(state *IssueMetadataState) error { Default: state.Title, } - title := "" - - err := prompt.SurveyAskOne(p, &title) + err := prompt.SurveyAskOne(p, state) if err != nil { return err } - state.Title = title - return nil } diff --git a/pkg/githubtemplate/github_template_test.go b/pkg/githubtemplate/github_template_test.go index d1c73fec7..c9f42f552 100644 --- a/pkg/githubtemplate/github_template_test.go +++ b/pkg/githubtemplate/github_template_test.go @@ -250,10 +250,10 @@ func TestFindLegacy(t *testing.T) { } got := FindLegacy(tt.args.rootDir, tt.args.name) - if got == nil { + if got == "" { t.Errorf("FindLegacy() = nil, want %v", tt.want) - } else if *got != tt.want { - t.Errorf("FindLegacy() = %v, want %v", *got, tt.want) + } else if got != tt.want { + t.Errorf("FindLegacy() = %v, want %v", got, tt.want) } }) os.RemoveAll(tmpdir) diff --git a/pkg/prompt/stubber.go b/pkg/prompt/stubber.go index ab08cd1ab..b1470bc0c 100644 --- a/pkg/prompt/stubber.go +++ b/pkg/prompt/stubber.go @@ -45,7 +45,7 @@ func InitAskStubber() (*AskStubber, func()) { count := as.Count as.Count += 1 if count >= len(as.Stubs) { - panic(fmt.Sprintf("more asks than stubs. most recent call: %v", qs)) + panic(fmt.Sprintf("more asks than stubs. most recent call: %#v", qs)) } // actually set response From b231e6c2cf81c4ee5830da8f5824b38dea0050b6 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 12 Nov 2020 12:15:48 -0800 Subject: [PATCH 6/7] use NewIssueState --- pkg/cmd/pr/create/create.go | 70 ++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 8dd21ca33..a9009b929 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -146,26 +146,9 @@ func createRun(opts *CreateOptions) (err error) { client := ctx.Client - var milestoneTitles []string - if opts.Milestone != "" { - milestoneTitles = []string{opts.Milestone} - } - - state := shared.IssueMetadataState{ - Type: shared.PRMetadata, - Reviewers: opts.Reviewers, - Assignees: opts.Assignees, - Labels: opts.Labels, - Projects: opts.Projects, - Milestones: milestoneTitles, - } - - var defaultsErr error - if opts.Autofill || !opts.TitleProvided || !opts.BodyProvided { - defaultsErr = computeDefaults(*ctx, &state) - if defaultsErr != nil { - return fmt.Errorf("could not compute title or body defaults: %w", defaultsErr) - } + state, err := NewIssueState(*ctx, *opts) + if err != nil { + return err } if opts.WebMode { @@ -177,7 +160,7 @@ func createRun(opts *CreateOptions) (err error) { if err != nil { return err } - return previewPR(*opts, *ctx, state) + return previewPR(*opts, *ctx, *state) } if opts.TitleProvided { @@ -211,10 +194,6 @@ func createRun(opts *CreateOptions) (err error) { cs.Cyan(ctx.HeadBranchLabel), cs.Cyan(ctx.BaseBranch), ghrepo.FullName(ctx.BaseRepo)) - if (state.Title == "" || state.Body == "") && defaultsErr != nil { - fmt.Fprintf(opts.IO.ErrOut, - "%s warning: could not compute title or body defaults: %s\n", cs.Yellow("!"), defaultsErr) - } } if opts.Autofill || (opts.TitleProvided && opts.BodyProvided) { @@ -222,11 +201,11 @@ func createRun(opts *CreateOptions) (err error) { if err != nil { return err } - return submitPR(*opts, *ctx, state) + return submitPR(*opts, *ctx, *state) } if !opts.TitleProvided { - err = shared.TitleSurvey(&state) + err = shared.TitleSurvey(state) if err != nil { return err } @@ -241,12 +220,12 @@ func createRun(opts *CreateOptions) (err error) { if !opts.BodyProvided { templateFiles, legacyTemplate := shared.FindTemplates(opts.RootDirOverride, "PULL_REQUEST_TEMPLATE") - templateContent, err = shared.TemplateSurvey(templateFiles, legacyTemplate, state) + templateContent, err = shared.TemplateSurvey(templateFiles, legacyTemplate, *state) if err != nil { return err } - err = shared.BodySurvey(&state, templateContent, editorCommand) + err = shared.BodySurvey(state, templateContent, editorCommand) if err != nil { return err } @@ -263,7 +242,7 @@ func createRun(opts *CreateOptions) (err error) { } if action == shared.MetadataAction { - err = shared.MetadataSurvey(opts.IO, client, ctx.BaseRepo, &state) + err = shared.MetadataSurvey(opts.IO, client, ctx.BaseRepo, state) if err != nil { return err } @@ -285,17 +264,17 @@ func createRun(opts *CreateOptions) (err error) { } if action == shared.PreviewAction { - return previewPR(*opts, *ctx, state) + return previewPR(*opts, *ctx, *state) } if action == shared.SubmitAction { - return submitPR(*opts, *ctx, state) + return submitPR(*opts, *ctx, *state) } return errors.New("expected to cancel, preview, or submit") } -func computeDefaults(ctx CreateContext, state *shared.IssueMetadataState) error { +func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState) error { baseRef := ctx.BaseTrackingBranch headRef := ctx.HeadBranch @@ -365,6 +344,31 @@ func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.Tr return nil } +func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { + var milestoneTitles []string + if opts.Milestone != "" { + milestoneTitles = []string{opts.Milestone} + } + + state := &shared.IssueMetadataState{ + Type: shared.PRMetadata, + Reviewers: opts.Reviewers, + Assignees: opts.Assignees, + Labels: opts.Labels, + Projects: opts.Projects, + Milestones: milestoneTitles, + } + + if opts.Autofill || !opts.TitleProvided || !opts.BodyProvided { + err := initDefaultTitleBody(ctx, state) + if err != nil { + return nil, fmt.Errorf("could not compute title or body defaults: %w", err) + } + } + + return state, nil +} + func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { httpClient, err := opts.HttpClient() if err != nil { From a686455fb6001018bf4b55a5356c36fc1e5eeb64 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 12 Nov 2020 12:17:37 -0800 Subject: [PATCH 7/7] add Draft to issue state --- pkg/cmd/pr/create/create.go | 5 +++-- pkg/cmd/pr/shared/survey.go | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index a9009b929..9e0180965 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -183,7 +183,7 @@ func createRun(opts *CreateOptions) (err error) { } message := "\nCreating pull request for %s into %s in %s\n\n" - if opts.IsDraft { + if state.Draft { message = "\nCreating draft pull request for %s into %s in %s\n\n" } @@ -357,6 +357,7 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata Labels: opts.Labels, Projects: opts.Projects, Milestones: milestoneTitles, + Draft: opts.IsDraft, } if opts.Autofill || !opts.TitleProvided || !opts.BodyProvided { @@ -538,7 +539,7 @@ func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataS params := map[string]interface{}{ "title": state.Title, "body": state.Body, - "draft": opts.IsDraft, + "draft": state.Draft, "baseRefName": ctx.BaseBranch, "headRefName": ctx.HeadBranchLabel, } diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 37d493d82..e6df8cf70 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -26,6 +26,8 @@ const ( type IssueMetadataState struct { Type metadataStateType + Draft bool + Body string Title string