diff --git a/.spr.yml b/.spr.yml index 8ec0f4da..58b12a49 100644 --- a/.spr.yml +++ b/.spr.yml @@ -1,6 +1,8 @@ githubRepoOwner: ejoffe githubRepoName: spr githubHost: github.com +githubRemote: origin +githubBranch: master requireChecks: true requireApproval: false mergeMethod: rebase diff --git a/config/config.go b/config/config.go index d8f1c3a4..26183b4c 100644 --- a/config/config.go +++ b/config/config.go @@ -30,6 +30,7 @@ type RepoConfig struct { MergeMethod string `default:"rebase" yaml:"mergeMethod"` MergeQueue bool `default:"false" yaml:"mergeQueue"` + PRTemplateType string `default:"stack" yaml:"prTemplateType"` PRTemplatePath string `yaml:"prTemplatePath,omitempty"` PRTemplateInsertStart string `yaml:"prTemplateInsertStart,omitempty"` PRTemplateInsertEnd string `yaml:"prTemplateInsertEnd,omitempty"` @@ -83,9 +84,21 @@ func DefaultConfig() *Config { cfg.User.LogGitCommands = false cfg.User.LogGitHubCalls = false + + // Normalize config (e.g., set PRTemplateType to "custom" if PRTemplatePath is provided) + cfg.Normalize() + return cfg } +// Normalize applies normalization rules to the config +// For example, if PRTemplatePath is provided, PRTemplateType should be set to "custom" +func (c *Config) Normalize() { + if c.Repo != nil && c.Repo.PRTemplatePath != "" { + c.Repo.PRTemplateType = "custom" + } +} + func (c Config) MergeMethod() (genclient.PullRequestMergeMethod, error) { var mergeMethod genclient.PullRequestMergeMethod var err error diff --git a/config/config_parser/config_parser.go b/config/config_parser/config_parser.go index 37150f08..2305b464 100644 --- a/config/config_parser/config_parser.go +++ b/config/config_parser/config_parser.go @@ -61,6 +61,10 @@ func ParseConfig(gitcmd git.GitInterface) *config.Config { rake.LoadSources(cfg.User, rake.YamlFileWriter(UserConfigFilePath())) } + + // Normalize config (e.g., set PRTemplateType to "custom" if PRTemplatePath is provided) + cfg.Normalize() + return cfg } diff --git a/config/config_test.go b/config/config_test.go index 62553dd5..b322c581 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -30,6 +30,7 @@ func TestDefaultConfig(t *testing.T) { RequireChecks: true, RequireApproval: true, MergeMethod: "rebase", + PRTemplateType: "stack", PRTemplatePath: "", PRTemplateInsertStart: "", PRTemplateInsertEnd: "", @@ -90,3 +91,48 @@ func TestMergeMethodHelper(t *testing.T) { assert.Empty(t, actual) }) } + +func TestNormalizeConfig(t *testing.T) { + t.Run("PRTemplatePath provided sets PRTemplateType to custom", func(t *testing.T) { + cfg := &Config{ + Repo: &RepoConfig{ + PRTemplateType: "stack", + PRTemplatePath: "/path/to/template.md", + }, + } + cfg.Normalize() + assert.Equal(t, "custom", cfg.Repo.PRTemplateType) + assert.Equal(t, "/path/to/template.md", cfg.Repo.PRTemplatePath) + }) + + t.Run("PRTemplatePath empty does not change PRTemplateType", func(t *testing.T) { + cfg := &Config{ + Repo: &RepoConfig{ + PRTemplateType: "stack", + PRTemplatePath: "", + }, + } + cfg.Normalize() + assert.Equal(t, "stack", cfg.Repo.PRTemplateType) + assert.Equal(t, "", cfg.Repo.PRTemplatePath) + }) + + t.Run("PRTemplatePath provided overrides existing PRTemplateType", func(t *testing.T) { + cfg := &Config{ + Repo: &RepoConfig{ + PRTemplateType: "why_what", + PRTemplatePath: "/custom/template.md", + }, + } + cfg.Normalize() + assert.Equal(t, "custom", cfg.Repo.PRTemplateType) + assert.Equal(t, "/custom/template.md", cfg.Repo.PRTemplatePath) + }) + + t.Run("DefaultConfig with PRTemplatePath sets PRTemplateType to custom", func(t *testing.T) { + cfg := DefaultConfig() + cfg.Repo.PRTemplatePath = "/path/to/template.md" + cfg.Normalize() + assert.Equal(t, "custom", cfg.Repo.PRTemplateType) + }) +} diff --git a/github/githubclient/client.go b/github/githubclient/client.go index f2b6b449..b59c811a 100644 --- a/github/githubclient/client.go +++ b/github/githubclient/client.go @@ -1,13 +1,11 @@ package githubclient import ( - "bytes" "context" "fmt" "net/url" "os" "path" - "path/filepath" "strings" "gopkg.in/yaml.v3" @@ -17,6 +15,7 @@ import ( "github.com/ejoffe/spr/github" "github.com/ejoffe/spr/github/githubclient/fezzik_types" "github.com/ejoffe/spr/github/githubclient/gen/genclient" + "github.com/ejoffe/spr/github/template/config_fetcher" "github.com/rs/zerolog/log" "golang.org/x/oauth2" ) @@ -384,22 +383,14 @@ func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, Str("FromBranch", headRefName).Str("ToBranch", baseRefName). Msg("CreatePullRequest") - body := formatBody(commit, info.PullRequests, c.config.Repo.ShowPrTitlesInStack) - if c.config.Repo.PRTemplatePath != "" { - pullRequestTemplate, err := readPRTemplate(gitcmd, c.config.Repo.PRTemplatePath) - if err != nil { - log.Fatal().Err(err).Msg("failed to read PR template") - } - body, err = insertBodyIntoPRTemplate(body, pullRequestTemplate, c.config.Repo, nil) - if err != nil { - log.Fatal().Err(err).Msg("failed to insert body into PR template") - } - } + templatizer := config_fetcher.PRTemplatizer(c.config, gitcmd) + + body := templatizer.Body(info, commit) resp, err := c.api.CreatePullRequest(ctx, genclient.CreatePullRequestInput{ RepositoryId: info.RepositoryID, BaseRefName: baseRefName, HeadRefName: headRefName, - Title: commit.Subject, + Title: templatizer.Title(info, commit), Body: &body, Draft: &c.config.User.CreateDraftPRs, }) @@ -427,113 +418,9 @@ func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, return pr } -func formatStackMarkdown(commit git.Commit, stack []*github.PullRequest, showPrTitlesInStack bool) string { - var buf bytes.Buffer - for i := len(stack) - 1; i >= 0; i-- { - isCurrent := stack[i].Commit == commit - var suffix string - if isCurrent { - suffix = " ⬅" - } else { - suffix = "" - } - var prTitle string - if showPrTitlesInStack { - prTitle = fmt.Sprintf("%s ", stack[i].Title) - } else { - prTitle = "" - } - - buf.WriteString(fmt.Sprintf("- %s#%d%s\n", prTitle, stack[i].Number, suffix)) - } - - return buf.String() -} - -func formatBody(commit git.Commit, stack []*github.PullRequest, showPrTitlesInStack bool) string { - if len(stack) <= 1 { - return strings.TrimSpace(commit.Body) - } - - if commit.Body == "" { - return fmt.Sprintf("**Stack**:\n%s", - addManualMergeNotice(formatStackMarkdown(commit, stack, showPrTitlesInStack))) - } - - return fmt.Sprintf("%s\n\n---\n\n**Stack**:\n%s", - commit.Body, - addManualMergeNotice(formatStackMarkdown(commit, stack, showPrTitlesInStack))) -} - -// Reads the specified PR template file and returns it as a string -func readPRTemplate(gitcmd git.GitInterface, templatePath string) (string, error) { - repoRootDir := gitcmd.RootDir() - fullTemplatePath := filepath.Clean(path.Join(repoRootDir, templatePath)) - pullRequestTemplateBytes, err := os.ReadFile(fullTemplatePath) - if err != nil { - return "", fmt.Errorf("%w: unable to read template %v", err, fullTemplatePath) - } - return string(pullRequestTemplateBytes), nil -} - -// insertBodyIntoPRTemplate inserts a text body into the given PR template and returns the result as a string. -// It uses the PRTemplateInsertStart and PRTemplateInsertEnd values defined in RepoConfig to determine where the body -// should be inserted in the PR template. If there are issues finding the correct place to insert the body -// an error will be returned. -// -// NOTE: on PR update, rather than using the PR template, it will use the existing PR body, which should have -// the PR template from the initial PR create. -func insertBodyIntoPRTemplate(body, prTemplate string, repo *config.RepoConfig, pr *github.PullRequest) (string, error) { - templateOrExistingPRBody := prTemplate - if pr != nil && pr.Body != "" { - templateOrExistingPRBody = pr.Body - } - - startPRTemplateSection, err := getSectionOfPRTemplate(templateOrExistingPRBody, repo.PRTemplateInsertStart, BeforeMatch) - if err != nil { - return "", fmt.Errorf("%w: PR template insert start = '%v'", err, repo.PRTemplateInsertStart) - } - - endPRTemplateSection, err := getSectionOfPRTemplate(templateOrExistingPRBody, repo.PRTemplateInsertEnd, AfterMatch) - if err != nil { - return "", fmt.Errorf("%w: PR template insert end = '%v'", err, repo.PRTemplateInsertEnd) - } - - return fmt.Sprintf("%v%v\n%v\n\n%v%v", startPRTemplateSection, repo.PRTemplateInsertStart, body, - repo.PRTemplateInsertEnd, endPRTemplateSection), nil -} - -const ( - BeforeMatch = iota - AfterMatch -) - -// getSectionOfPRTemplate searches text for a matching searchString and will return the text before or after the -// match as a string. If there are no matches or more than one match is found, an error will be returned. -func getSectionOfPRTemplate(text, searchString string, returnMatch int) (string, error) { - split := strings.Split(text, searchString) - switch len(split) { - case 2: - if returnMatch == BeforeMatch { - return split[0], nil - } else if returnMatch == AfterMatch { - return split[1], nil - } - return "", fmt.Errorf("invalid enum value") - case 1: - return "", fmt.Errorf("no matches found") - default: - return "", fmt.Errorf("multiple matches found") - } -} - -func addManualMergeNotice(body string) string { - return body + "\n\n" + - "⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). " + - "Do not merge manually using the UI - doing so may have unexpected results.*" -} - -func (c *client) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, pullRequests []*github.PullRequest, pr *github.PullRequest, commit git.Commit, prevCommit *git.Commit) { +func (c *client) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, + info *github.GitHubInfo, pullRequests []*github.PullRequest, pr *github.PullRequest, + commit git.Commit, prevCommit *git.Commit) { if c.config.User.LogGitHubCalls { fmt.Printf("> github update %d : %s\n", pr.Number, pr.Title) @@ -548,34 +435,23 @@ func (c *client) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, Str("FromBranch", pr.FromBranch).Str("ToBranch", baseRefName). Interface("PR", pr).Msg("UpdatePullRequest") - body := formatBody(commit, pullRequests, c.config.Repo.ShowPrTitlesInStack) - if c.config.Repo.PRTemplatePath != "" { - pullRequestTemplate, err := readPRTemplate(gitcmd, c.config.Repo.PRTemplatePath) - if err != nil { - log.Fatal().Err(err).Msg("failed to read PR template") - } - body, err = insertBodyIntoPRTemplate(body, pullRequestTemplate, c.config.Repo, pr) - if err != nil { - log.Fatal().Err(err).Msg("failed to insert body into PR template") - } - } - title := &commit.Subject - + templatizer := config_fetcher.PRTemplatizer(c.config, gitcmd) + title := templatizer.Title(info, commit) + body := templatizer.Body(info, commit) input := genclient.UpdatePullRequestInput{ PullRequestId: pr.ID, - Title: title, + Title: &title, Body: &body, } - - if !pr.InQueue { - input.BaseRefName = &baseRefName - } - if c.config.User.PreserveTitleAndBody { input.Title = nil input.Body = nil } + if !pr.InQueue { + input.BaseRefName = &baseRefName + } + _, err := c.api.UpdatePullRequest(ctx, input) if err != nil { diff --git a/github/githubclient/client_test.go b/github/githubclient/client_test.go index be62a82b..c2cbd588 100644 --- a/github/githubclient/client_test.go +++ b/github/githubclient/client_test.go @@ -1,7 +1,6 @@ package githubclient import ( - "strings" "testing" "github.com/ejoffe/spr/config" @@ -529,299 +528,3 @@ func TestMatchPullRequestStack(t *testing.T) { }) } } - -func TestFormatPullRequestBody(t *testing.T) { - simpleCommit := git.Commit{ - CommitID: "abc123", - CommitHash: "abcdef123456", - } - descriptiveCommit := git.Commit{ - CommitID: "def456", - CommitHash: "ghijkl7890", - Body: `This body describes my nice PR. -It even includes some **markdown** formatting.`} - - tests := []struct { - description string - commit git.Commit - stack []*github.PullRequest - }{ - { - description: "", - commit: git.Commit{}, - stack: []*github.PullRequest{}, - }, - { - description: `This body describes my nice PR. -It even includes some **markdown** formatting.`, - commit: descriptiveCommit, - stack: []*github.PullRequest{ - {Number: 2, Commit: descriptiveCommit}, - }, - }, - { - description: `This body describes my nice PR. -It even includes some **markdown** formatting. - ---- - -**Stack**: -- #2 ⬅ -- #1 - - -⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, - commit: descriptiveCommit, - stack: []*github.PullRequest{ - {Number: 1, Commit: simpleCommit}, - {Number: 2, Commit: descriptiveCommit}, - }, - }, - } - - for _, tc := range tests { - body := formatBody(tc.commit, tc.stack, false) - if body != tc.description { - t.Fatalf("expected: '%v', actual: '%v'", tc.description, body) - } - } -} - -func TestFormatPullRequestBody_ShowPrTitle(t *testing.T) { - simpleCommit := git.Commit{ - CommitID: "abc123", - CommitHash: "abcdef123456", - } - descriptiveCommit := git.Commit{ - CommitID: "def456", - CommitHash: "ghijkl7890", - Body: `This body describes my nice PR. -It even includes some **markdown** formatting.`} - - tests := []struct { - description string - commit git.Commit - stack []*github.PullRequest - }{ - { - description: "", - commit: git.Commit{}, - stack: []*github.PullRequest{}, - }, - { - description: `This body describes my nice PR. -It even includes some **markdown** formatting.`, - commit: descriptiveCommit, - stack: []*github.PullRequest{ - {Number: 2, Commit: descriptiveCommit}, - }, - }, - { - description: `This body describes my nice PR. -It even includes some **markdown** formatting. - ---- - -**Stack**: -- Title B #2 ⬅ -- Title A #1 - - -⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, - commit: descriptiveCommit, - stack: []*github.PullRequest{ - {Number: 1, Commit: simpleCommit, Title: "Title A"}, - {Number: 2, Commit: descriptiveCommit, Title: "Title B"}, - }, - }, - } - - for _, tc := range tests { - body := formatBody(tc.commit, tc.stack, true) - if body != tc.description { - t.Fatalf("expected: '%v', actual: '%v'", tc.description, body) - } - } -} - -func TestInsertBodyIntoPRTemplateHappyPath(t *testing.T) { - tests := []struct { - name string - body string - pullRequestTemplate string - repo *config.RepoConfig - pr *github.PullRequest - expected string - }{ - { - name: "create PR", - body: "inserted body", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "## Description", - PRTemplateInsertEnd: "## Checklist", - }, - pr: nil, - expected: ` -## Related Issues - - -## Description -inserted body - -## Checklist -- [ ] My code follows the style guidelines of this project`, - }, - { - name: "update PR", - body: "updated description", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "## Description", - PRTemplateInsertEnd: "## Checklist", - }, - pr: &github.PullRequest{ - Body: ` -## Related Issues -* Issue #1234 - -## Description -original description - -## Checklist -- [x] My code follows the style guidelines of this project`, - }, - expected: ` -## Related Issues -* Issue #1234 - -## Description -updated description - -## Checklist -- [x] My code follows the style guidelines of this project`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - body, _ := insertBodyIntoPRTemplate(tt.body, tt.pullRequestTemplate, tt.repo, tt.pr) - if body != tt.expected { - t.Fatalf("expected: '%v', actual: '%v'", tt.expected, body) - } - }) - } -} - -func TestInsertBodyIntoPRTemplateErrors(t *testing.T) { - tests := []struct { - name string - body string - pullRequestTemplate string - repo *config.RepoConfig - pr *github.PullRequest - expected string - }{ - { - name: "no match insert start", - body: "inserted body", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "does not exist", - PRTemplateInsertEnd: "## Checklist", - }, - pr: nil, - expected: "no matches found: PR template insert start", - }, - { - name: "no match insert end", - body: "inserted body", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "## Description", - PRTemplateInsertEnd: "does not exist", - }, - pr: nil, - expected: "no matches found: PR template insert end", - }, - { - name: "multiple many matches insert start", - body: "inserted body", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "duplicate", - PRTemplateInsertEnd: "## Checklist", - }, - pr: nil, - expected: "multiple matches found: PR template insert start", - }, - { - name: "multiple many matches insert end", - body: "inserted body", - pullRequestTemplate: ` -## Related Issues - - -## Description - - -## Checklist -- [ ] My code follows the style guidelines of this project`, - repo: &config.RepoConfig{ - PRTemplateInsertStart: "## Description", - PRTemplateInsertEnd: "duplicate", - }, - pr: nil, - expected: "multiple matches found: PR template insert end", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := insertBodyIntoPRTemplate(tt.body, tt.pullRequestTemplate, tt.repo, tt.pr) - if !strings.Contains(err.Error(), tt.expected) { - t.Fatalf("expected: '%v', actual: '%v'", tt.expected, err.Error()) - } - }) - } -} diff --git a/github/interface.go b/github/interface.go index 5e708fd3..18c25da6 100644 --- a/github/interface.go +++ b/github/interface.go @@ -18,7 +18,7 @@ type GitHubInterface interface { CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, info *GitHubInfo, commit git.Commit, prevCommit *git.Commit) *PullRequest // UpdatePullRequest updates a pull request with current commit - UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, pullRequests []*PullRequest, pr *PullRequest, commit git.Commit, prevCommit *git.Commit) + UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, info *GitHubInfo, pullRequests []*PullRequest, pr *PullRequest, commit git.Commit, prevCommit *git.Commit) // AddReviewers adds a reviewer to the given pull request AddReviewers(ctx context.Context, pr *PullRequest, userIDs []string) diff --git a/github/mockclient/mockclient.go b/github/mockclient/mockclient.go index aad3aa5e..bda9399d 100644 --- a/github/mockclient/mockclient.go +++ b/github/mockclient/mockclient.go @@ -82,7 +82,7 @@ func (c *MockClient) CreatePullRequest(ctx context.Context, gitcmd git.GitInterf } } -func (c *MockClient) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, pullRequests []*github.PullRequest, pr *github.PullRequest, commit git.Commit, prevCommit *git.Commit) { +func (c *MockClient) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, info *github.GitHubInfo, pullRequests []*github.PullRequest, pr *github.PullRequest, commit git.Commit, prevCommit *git.Commit) { fmt.Printf("HUB: UpdatePullRequest\n") c.verifyExpectation(expectation{ op: updatePullRequestOP, diff --git a/github/template/config_fetcher/config.go b/github/template/config_fetcher/config.go new file mode 100644 index 00000000..b6e6b423 --- /dev/null +++ b/github/template/config_fetcher/config.go @@ -0,0 +1,26 @@ +package config_fetcher + +import ( + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github/template" + "github.com/ejoffe/spr/github/template/template_basic" + "github.com/ejoffe/spr/github/template/template_custom" + "github.com/ejoffe/spr/github/template/template_stack" + "github.com/ejoffe/spr/github/template/template_why_what" +) + +func PRTemplatizer(c *config.Config, gitcmd git.GitInterface) template.PRTemplatizer { + switch c.Repo.PRTemplateType { + case "stack": + return template_stack.NewStackTemplatizer(c.Repo.ShowPrTitlesInStack) + case "basic": + return template_basic.NewBasicTemplatizer() + case "why_what": + return template_why_what.NewWhyWhatTemplatizer() + case "custom": + return template_custom.NewCustomTemplatizer(c.Repo, gitcmd) + default: + return template_basic.NewBasicTemplatizer() + } +} diff --git a/github/template/helpers.go b/github/template/helpers.go new file mode 100644 index 00000000..6926c2be --- /dev/null +++ b/github/template/helpers.go @@ -0,0 +1,45 @@ +package template + +import ( + "bytes" + "fmt" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" +) + +/* +func AddManualMergeNotice(body string) string { + return body + "\n\n" + + "⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). " + + "Do not merge manually using the UI - doing so may have unexpected results.*" +} +*/ + +func ManualMergeNotice() string { + return "⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). " + + "Do not merge manually using the UI - doing so may have unexpected results.*" +} + +func FormatStackMarkdown(commit git.Commit, stack []*github.PullRequest, showPrTitlesInStack bool) string { + var buf bytes.Buffer + for i := len(stack) - 1; i >= 0; i-- { + isCurrent := stack[i].Commit == commit + var suffix string + if isCurrent { + suffix = " ⬅" + } else { + suffix = "" + } + var prTitle string + if showPrTitlesInStack { + prTitle = fmt.Sprintf("%s ", stack[i].Title) + } else { + prTitle = "" + } + + buf.WriteString(fmt.Sprintf("- %s#%d%s\n", prTitle, stack[i].Number, suffix)) + } + + return buf.String() +} diff --git a/github/template/interface.go b/github/template/interface.go new file mode 100644 index 00000000..578f0971 --- /dev/null +++ b/github/template/interface.go @@ -0,0 +1,11 @@ +package template + +import ( + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" +) + +type PRTemplatizer interface { + Title(info *github.GitHubInfo, commit git.Commit) string + Body(info *github.GitHubInfo, commit git.Commit) string +} diff --git a/github/template/template_basic/template.go b/github/template/template_basic/template.go new file mode 100644 index 00000000..3ccf5ab2 --- /dev/null +++ b/github/template/template_basic/template.go @@ -0,0 +1,24 @@ +package template_basic + +import ( + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/ejoffe/spr/github/template" +) + +type BasicTemplatizer struct{} + +func NewBasicTemplatizer() *BasicTemplatizer { + return &BasicTemplatizer{} +} + +func (t *BasicTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + return commit.Subject +} + +func (t *BasicTemplatizer) Body(info *github.GitHubInfo, commit git.Commit) string { + body := commit.Body + body += "\n\n" + body += template.ManualMergeNotice() + return body +} diff --git a/github/template/template_basic/template_test.go b/github/template/template_basic/template_test.go new file mode 100644 index 00000000..05b66326 --- /dev/null +++ b/github/template/template_basic/template_test.go @@ -0,0 +1,377 @@ +package template_basic + +import ( + "strings" + "testing" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/stretchr/testify/assert" +) + +func TestTitle(t *testing.T) { + templatizer := &BasicTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + want string + }{ + { + name: "simple subject", + commit: git.Commit{ + Subject: "Fix bug in authentication", + Body: "Some body text", + }, + want: "Fix bug in authentication", + }, + { + name: "empty subject", + commit: git.Commit{ + Subject: "", + Body: "Some body text", + }, + want: "", + }, + { + name: "subject with special characters", + commit: git.Commit{ + Subject: "Add feature: user authentication (WIP)", + Body: "Some body text", + }, + want: "Add feature: user authentication (WIP)", + }, + { + name: "subject with newline", + commit: git.Commit{ + Subject: "Fix bug\nwith newline", + Body: "Some body text", + }, + want: "Fix bug\nwith newline", + }, + { + name: "long subject", + commit: git.Commit{ + Subject: "This is a very long commit subject that might be used in some cases where developers write detailed commit messages", + Body: "Some body text", + }, + want: "This is a very long commit subject that might be used in some cases where developers write detailed commit messages", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Title(info, tt.commit) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestBody(t *testing.T) { + templatizer := &BasicTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + wantContains []string // strings that should be in the output + wantSuffix string // expected suffix (manual merge notice) + }{ + { + name: "empty body", + commit: git.Commit{ + Subject: "Test commit", + Body: "", + }, + wantContains: []string{ + "⚠️", + "Part of a stack created by [spr]", + "Do not merge manually using the UI", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "simple body", + commit: git.Commit{ + Subject: "Test commit", + Body: "This is a simple commit body", + }, + wantContains: []string{ + "This is a simple commit body", + "⚠️", + "Part of a stack created by [spr]", + "Do not merge manually using the UI", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with newlines", + commit: git.Commit{ + Subject: "Test commit", + Body: "Line 1\nLine 2\nLine 3", + }, + wantContains: []string{ + "Line 1", + "Line 2", + "Line 3", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with multiple paragraphs", + commit: git.Commit{ + Subject: "Test commit", + Body: "First paragraph\n\nSecond paragraph\n\nThird paragraph", + }, + wantContains: []string{ + "First paragraph", + "Second paragraph", + "Third paragraph", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with trailing newline", + commit: git.Commit{ + Subject: "Test commit", + Body: "Commit body with trailing newline\n", + }, + wantContains: []string{ + "Commit body with trailing newline", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with markdown", + commit: git.Commit{ + Subject: "Test commit", + Body: "# Header\n\n- List item 1\n- List item 2\n\n**Bold text**", + }, + wantContains: []string{ + "# Header", + "List item 1", + "List item 2", + "**Bold text**", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with special characters", + commit: git.Commit{ + Subject: "Test commit", + Body: "Body with special chars: @#$%^&*()[]{}|\\/<>?", + }, + wantContains: []string{ + "Body with special chars: @#$%^&*()[]{}|\\/<>?", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + { + name: "body with unicode characters", + commit: git.Commit{ + Subject: "Test commit", + Body: "Body with unicode: 你好世界 🌍 🚀", + }, + wantContains: []string{ + "Body with unicode: 你好世界 🌍 🚀", + "⚠️", + "Part of a stack created by [spr]", + }, + wantSuffix: "\n\n⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Body(info, tt.commit) + + // Verify all expected strings are present + for _, wantStr := range tt.wantContains { + assert.Contains(t, got, wantStr, "Expected output to contain: %s", wantStr) + } + + // Verify the manual merge notice is appended correctly + assert.True(t, strings.HasSuffix(got, tt.wantSuffix), + "Expected body to end with manual merge notice. Got suffix: %s", + got[max(0, len(got)-len(tt.wantSuffix)):]) + + // Verify the original body content is preserved at the start + if tt.commit.Body != "" { + // The body should start with the original commit body + expectedStart := tt.commit.Body + // If body doesn't end with newline, the notice adds \n\n, so we need to account for that + if !strings.HasSuffix(tt.commit.Body, "\n") { + assert.True(t, strings.HasPrefix(got, expectedStart), + "Expected body to start with original commit body") + } + } + }) + } +} + +func TestBodyManualMergeNoticeFormat(t *testing.T) { + templatizer := &BasicTemplatizer{} + info := &github.GitHubInfo{} + + commit := git.Commit{ + Subject: "Test commit", + Body: "Test body", + } + + result := templatizer.Body(info, commit) + + // Verify the exact format of the manual merge notice + expectedNotice := "⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*" + + // Should end with the notice + assert.True(t, strings.HasSuffix(result, "\n\n"+expectedNotice)) + + // Should contain the warning emoji + assert.Contains(t, result, "⚠️") + + // Should contain the spr link + assert.Contains(t, result, "[spr](https://github.com/ejoffe/spr)") + + // Should contain the warning message + assert.Contains(t, result, "Do not merge manually using the UI") +} + +func TestBodyPreservesOriginalContent(t *testing.T) { + templatizer := &BasicTemplatizer{} + info := &github.GitHubInfo{} + + testCases := []struct { + name string + body string + }{ + { + name: "preserves exact body content", + body: "Exact body content to preserve", + }, + { + name: "preserves body with whitespace", + body: " Body with whitespace ", + }, + { + name: "preserves body with newlines", + body: "Line 1\nLine 2\n\nLine 3", + }, + { + name: "preserves empty body", + body: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + commit := git.Commit{ + Subject: "Test", + Body: tc.body, + } + + result := templatizer.Body(info, commit) + + // The result should contain the original body (if not empty) + if tc.body != "" { + // For non-empty bodies, the original content should appear before the notice + noticeStart := strings.Index(result, "⚠️") + if noticeStart > 0 { + bodyPart := (result)[:noticeStart] + // Remove the trailing \n\n that separates body from notice + bodyPart = strings.TrimSuffix(bodyPart, "\n\n") + assert.Equal(t, tc.body, bodyPart, "Original body content should be preserved") + } + } else { + // For empty body, result should start with the notice (after \n\n) + assert.True(t, strings.HasPrefix(result, "\n\n⚠️") || strings.HasPrefix(result, "⚠️")) + } + }) + } +} + +func TestBodyWithRealWorldExamples(t *testing.T) { + templatizer := &BasicTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + }{ + { + name: "typical feature commit", + commit: git.Commit{ + Subject: "Add user authentication", + Body: "Implemented JWT-based authentication for API endpoints. Users can now login and receive tokens.", + }, + }, + { + name: "bug fix commit", + commit: git.Commit{ + Subject: "Fix memory leak in cache", + Body: "Fixed issue where cache entries were not being properly cleaned up, causing memory to grow over time.", + }, + }, + { + name: "documentation commit", + commit: git.Commit{ + Subject: "Update README", + Body: "Added installation instructions and usage examples.", + }, + }, + { + name: "multi-line detailed commit", + commit: git.Commit{ + Subject: "Refactor database layer", + Body: `This commit refactors the database layer to improve performance. + +Changes: +- Added connection pooling +- Optimized query execution +- Added caching layer + +Performance improvements: +- 50% reduction in query time +- 30% reduction in memory usage`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := templatizer.Body(info, tt.commit) + + // Should contain the original body content + assert.Contains(t, result, tt.commit.Body) + + // Should contain the manual merge notice + assert.Contains(t, result, "⚠️") + assert.Contains(t, result, "Part of a stack created by [spr]") + assert.Contains(t, result, "Do not merge manually using the UI") + + // Should not be empty + assert.NotEmpty(t, result) + + // Should end with the notice + assert.True(t, strings.HasSuffix(result, "Do not merge manually using the UI - doing so may have unexpected results.*")) + }) + } +} + +// Helper function for max (since Go 1.18+ has it in math package, but for compatibility) +func max(a, b int) int { + if a > b { + return a + } + return b +} diff --git a/github/template/template_custom/template.go b/github/template/template_custom/template.go new file mode 100644 index 00000000..2b27ffc8 --- /dev/null +++ b/github/template/template_custom/template.go @@ -0,0 +1,130 @@ +package template_custom + +import ( + "fmt" + "os" + "path" + "path/filepath" + "strings" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/ejoffe/spr/github/template" + "github.com/rs/zerolog/log" +) + +type CustomTemplatizer struct { + repoConfig *config.RepoConfig + gitcmd git.GitInterface +} + +func NewCustomTemplatizer( + repoConfig *config.RepoConfig, + gitcmd git.GitInterface, +) *CustomTemplatizer { + return &CustomTemplatizer{ + repoConfig: repoConfig, + gitcmd: gitcmd, + } +} + +func (t *CustomTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + return commit.Subject +} + +func (t *CustomTemplatizer) Body(info *github.GitHubInfo, commit git.Commit) string { + body := t.formatBody(commit, info.PullRequests) + pullRequestTemplate, err := t.readPRTemplate() + if err != nil { + log.Fatal().Err(err).Msg("failed to read PR template") + } + body, err = t.insertBodyIntoPRTemplate(body, pullRequestTemplate, nil) + if err != nil { + log.Fatal().Err(err).Msg("failed to insert body into PR template") + } + return body +} + +func (t *CustomTemplatizer) formatBody(commit git.Commit, stack []*github.PullRequest) string { + + if len(stack) <= 1 { + return strings.TrimSpace(commit.Body) + } + + if commit.Body == "" { + return fmt.Sprintf( + "**Stack**:\n%s\n%s", + template.FormatStackMarkdown(commit, stack, t.repoConfig.ShowPrTitlesInStack), + template.ManualMergeNotice(), + ) + } + + return fmt.Sprintf("%s\n\n---\n\n**Stack**:\n%s\n%s", + commit.Body, + template.FormatStackMarkdown(commit, stack, t.repoConfig.ShowPrTitlesInStack), + template.ManualMergeNotice(), + ) +} + +// Reads the specified PR template file and returns it as a string +func (t *CustomTemplatizer) readPRTemplate() (string, error) { + repoRootDir := t.gitcmd.RootDir() + fullTemplatePath := filepath.Clean(path.Join(repoRootDir, t.repoConfig.PRTemplatePath)) + pullRequestTemplateBytes, err := os.ReadFile(fullTemplatePath) + if err != nil { + return "", fmt.Errorf("%w: unable to read template %v", err, fullTemplatePath) + } + return string(pullRequestTemplateBytes), nil +} + +// insertBodyIntoPRTemplate inserts a text body into the given PR template and returns the result as a string. +// It uses the PRTemplateInsertStart and PRTemplateInsertEnd values defined in RepoConfig to determine where the body +// should be inserted in the PR template. If there are issues finding the correct place to insert the body +// an error will be returned. +// +// NOTE: on PR update, rather than using the PR template, it will use the existing PR body, which should have +// the PR template from the initial PR create. +func (t *CustomTemplatizer) insertBodyIntoPRTemplate(body, prTemplate string, pr *github.PullRequest) (string, error) { + templateOrExistingPRBody := prTemplate + if pr != nil && pr.Body != "" { + templateOrExistingPRBody = pr.Body + } + + startPRTemplateSection, err := getSectionOfPRTemplate(templateOrExistingPRBody, t.repoConfig.PRTemplateInsertStart, BeforeMatch) + if err != nil { + return "", fmt.Errorf("%w: PR template insert start = '%v'", err, t.repoConfig.PRTemplateInsertStart) + } + + endPRTemplateSection, err := getSectionOfPRTemplate(templateOrExistingPRBody, t.repoConfig.PRTemplateInsertEnd, AfterMatch) + if err != nil { + return "", fmt.Errorf("%w: PR template insert end = '%v'", err, t.repoConfig.PRTemplateInsertEnd) + } + + return fmt.Sprintf("%v%v\n%v\n\n%v%v", startPRTemplateSection, t.repoConfig.PRTemplateInsertStart, body, + t.repoConfig.PRTemplateInsertEnd, endPRTemplateSection), nil +} + +const ( + BeforeMatch = iota + AfterMatch +) + +// getSectionOfPRTemplate searches text for a matching searchString and will return the text before or after the +// match as a string. If there are no matches or more than one match is found, an error will be returned. +func getSectionOfPRTemplate(text, searchString string, returnMatch int) (string, error) { + split := strings.Split(text, searchString) + switch len(split) { + case 2: + if returnMatch == BeforeMatch { + return split[0], nil + } else if returnMatch == AfterMatch { + return split[1], nil + } + return "", fmt.Errorf("invalid enum value") + case 1: + return "", fmt.Errorf("no matches found") + default: + return "", fmt.Errorf("multiple matches found") + } +} diff --git a/github/template/template_custom/template_test.go b/github/template/template_custom/template_test.go new file mode 100644 index 00000000..1d37f3d4 --- /dev/null +++ b/github/template/template_custom/template_test.go @@ -0,0 +1,516 @@ +package template_custom + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockGit is a simple mock implementation of git.GitInterface for testing +type mockGit struct { + rootDir string +} + +func (m *mockGit) GitWithEditor(args string, output *string, editorCmd string) error { + return nil +} + +func (m *mockGit) Git(args string, output *string) error { + return nil +} + +func (m *mockGit) MustGit(args string, output *string) { +} + +func (m *mockGit) RootDir() string { + return m.rootDir +} + +func (m *mockGit) DeleteRemoteBranch(ctx context.Context, branch string) error { + return nil +} + +func TestTitle(t *testing.T) { + repoConfig := &config.RepoConfig{} + gitcmd := &mockGit{rootDir: "/tmp"} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + want string + }{ + { + name: "simple subject", + commit: git.Commit{ + Subject: "Fix bug in authentication", + Body: "Some body text", + }, + want: "Fix bug in authentication", + }, + { + name: "empty subject", + commit: git.Commit{ + Subject: "", + Body: "Some body text", + }, + want: "", + }, + { + name: "subject with special characters", + commit: git.Commit{ + Subject: "Add feature: user authentication (WIP)", + Body: "Some body text", + }, + want: "Add feature: user authentication (WIP)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Title(info, tt.commit) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFormatBody(t *testing.T) { + repoConfig := &config.RepoConfig{ + ShowPrTitlesInStack: false, + } + gitcmd := &mockGit{rootDir: "/tmp"} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + tests := []struct { + name string + commit git.Commit + stack []*github.PullRequest + contains []string + }{ + { + name: "single commit in stack", + commit: git.Commit{ + Subject: "Test commit", + Body: "Commit body", + }, + stack: []*github.PullRequest{ + {Number: 1, Commit: git.Commit{CommitID: "commit1"}}, + }, + contains: []string{"Commit body"}, + }, + { + name: "empty stack", + commit: git.Commit{ + Subject: "Test commit", + Body: "Commit body", + }, + stack: []*github.PullRequest{}, + contains: []string{"Commit body"}, + }, + { + name: "multiple commits with body", + commit: git.Commit{ + Subject: "Test commit", + Body: "Commit body text", + }, + stack: []*github.PullRequest{ + {Number: 1, Commit: git.Commit{CommitID: "commit1"}}, + {Number: 2, Commit: git.Commit{CommitID: "commit2"}}, + }, + contains: []string{ + "Commit body text", + "---", + "**Stack**:", + "#1", + "#2", + "⚠️", + "Part of a stack created by [spr]", + }, + }, + { + name: "multiple commits with empty body", + commit: git.Commit{ + Subject: "Test commit", + Body: "", + }, + stack: []*github.PullRequest{ + {Number: 1, Commit: git.Commit{CommitID: "commit1"}}, + {Number: 2, Commit: git.Commit{CommitID: "commit2"}}, + }, + contains: []string{ + "**Stack**:", + "#1", + "#2", + "⚠️", + }, + }, + { + name: "stack with PR titles", + commit: git.Commit{ + Subject: "Test commit", + Body: "Commit body", + }, + stack: []*github.PullRequest{ + {Number: 1, Title: "First PR", Commit: git.Commit{CommitID: "commit1"}}, + {Number: 2, Title: "Second PR", Commit: git.Commit{CommitID: "commit2"}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := templatizer.formatBody(tt.commit, tt.stack) + + for _, wantStr := range tt.contains { + assert.Contains(t, result, wantStr, "Expected output to contain: %s", wantStr) + } + + // For single commit or empty stack, body should be trimmed + if len(tt.stack) <= 1 { + assert.Equal(t, strings.TrimSpace(tt.commit.Body), result) + } + }) + } +} + +func TestFormatBodyWithPRTitles(t *testing.T) { + repoConfig := &config.RepoConfig{ + ShowPrTitlesInStack: true, + } + gitcmd := &mockGit{rootDir: "/tmp"} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + commit := git.Commit{ + Subject: "Test commit", + Body: "Commit body", + } + stack := []*github.PullRequest{ + {Number: 1, Title: "First PR", Commit: git.Commit{CommitID: "commit1"}}, + {Number: 2, Title: "Second PR", Commit: git.Commit{CommitID: "commit2"}}, + } + + result := templatizer.formatBody(commit, stack) + + assert.Contains(t, result, "First PR #1") + assert.Contains(t, result, "Second PR #2") +} + +func TestReadPRTemplate(t *testing.T) { + // Create a temporary directory and file + tmpDir := t.TempDir() + templatePath := filepath.Join(tmpDir, "pr_template.md") + templateContent := "# PR Template\n\n\n\n## Additional Notes\n" + + err := os.WriteFile(templatePath, []byte(templateContent), 0644) + require.NoError(t, err) + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + result, err := templatizer.readPRTemplate() + require.NoError(t, err) + assert.Equal(t, templateContent, result) +} + +func TestReadPRTemplateNotFound(t *testing.T) { + tmpDir := t.TempDir() + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "nonexistent_template.md", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + _, err := templatizer.readPRTemplate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "unable to read template") +} + +func TestReadPRTemplateWithSubdirectory(t *testing.T) { + tmpDir := t.TempDir() + subDir := filepath.Join(tmpDir, "templates") + err := os.MkdirAll(subDir, 0755) + require.NoError(t, err) + + templatePath := filepath.Join(subDir, "pr_template.md") + templateContent := "Template in subdirectory" + + err = os.WriteFile(templatePath, []byte(templateContent), 0644) + require.NoError(t, err) + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "templates/pr_template.md", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + result, err := templatizer.readPRTemplate() + require.NoError(t, err) + assert.Equal(t, templateContent, result) +} + +func TestGetSectionOfPRTemplate(t *testing.T) { + tests := []struct { + name string + text string + searchString string + matchType int + expected string + expectError bool + }{ + { + name: "before match", + text: "BeforeAfter", + searchString: "", + matchType: BeforeMatch, + expected: "Before", + expectError: false, + }, + { + name: "after match", + text: "BeforeAfter", + searchString: "", + matchType: AfterMatch, + expected: "After", + expectError: false, + }, + { + name: "no match found", + text: "Some text without marker", + searchString: "", + matchType: BeforeMatch, + expectError: true, + }, + { + name: "multiple matches", + text: "BeforeMiddleAfter", + searchString: "", + matchType: BeforeMatch, + expectError: true, + }, + { + name: "empty search string", + text: "Some text", + searchString: "", + matchType: BeforeMatch, + expectError: true, + }, + { + name: "match at start", + text: "Rest of text", + searchString: "", + matchType: BeforeMatch, + expected: "", + expectError: false, + }, + { + name: "match at end", + text: "Text before", + searchString: "", + matchType: AfterMatch, + expected: "", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := getSectionOfPRTemplate(tt.text, tt.searchString, tt.matchType) + + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + +func TestInsertBodyIntoPRTemplate(t *testing.T) { + tmpDir := t.TempDir() + templatePath := filepath.Join(tmpDir, "pr_template.md") + prTemplate := "# PR Template\n\n\n\n\n\n## Additional Notes\n" + + err := os.WriteFile(templatePath, []byte(prTemplate), 0644) + require.NoError(t, err) + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + PRTemplateInsertStart: "", + PRTemplateInsertEnd: "", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + body := "This is the commit body" + result, err := templatizer.insertBodyIntoPRTemplate(body, prTemplate, nil) + require.NoError(t, err) + + expected := "# PR Template\n\n\nThis is the commit body\n\n\n\n## Additional Notes\n" + assert.Equal(t, expected, result) +} + +func TestInsertBodyIntoPRTemplateWithExistingPR(t *testing.T) { + tmpDir := t.TempDir() + templatePath := filepath.Join(tmpDir, "pr_template.md") + prTemplate := "# PR Template\n\n\n\n\n" + + err := os.WriteFile(templatePath, []byte(prTemplate), 0644) + require.NoError(t, err) + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + PRTemplateInsertStart: "", + PRTemplateInsertEnd: "", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + body := "Updated commit body" + existingPRBody := "# PR Template\n\n\nOld body\n\n\n" + + result, err := templatizer.insertBodyIntoPRTemplate(body, prTemplate, &github.PullRequest{ + Body: existingPRBody, + }) + require.NoError(t, err) + + // Should use existing PR body instead of template + expected := "# PR Template\n\n\nUpdated commit body\n\n\n" + assert.Equal(t, expected, result) +} + +func TestInsertBodyIntoPRTemplateMissingStartMarker(t *testing.T) { + tmpDir := t.TempDir() + prTemplate := "# PR Template\n\n\n" + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + PRTemplateInsertStart: "", + PRTemplateInsertEnd: "", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + body := "Commit body" + _, err := templatizer.insertBodyIntoPRTemplate(body, prTemplate, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "PR template insert start") +} + +func TestInsertBodyIntoPRTemplateMissingEndMarker(t *testing.T) { + tmpDir := t.TempDir() + prTemplate := "# PR Template\n\n\n" + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + PRTemplateInsertStart: "", + PRTemplateInsertEnd: "", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + body := "Commit body" + _, err := templatizer.insertBodyIntoPRTemplate(body, prTemplate, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "PR template insert end") +} + +func TestInsertBodyIntoPRTemplateMultipleStartMarkers(t *testing.T) { + tmpDir := t.TempDir() + prTemplate := "# PR Template\n\n\n\n\n" + + repoConfig := &config.RepoConfig{ + PRTemplatePath: "pr_template.md", + PRTemplateInsertStart: "", + PRTemplateInsertEnd: "", + } + gitcmd := &mockGit{rootDir: tmpDir} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + body := "Commit body" + _, err := templatizer.insertBodyIntoPRTemplate(body, prTemplate, nil) + assert.Error(t, err) +} + +func TestNewCustomTemplatizer(t *testing.T) { + repoConfig := &config.RepoConfig{ + PRTemplatePath: "template.md", + } + gitcmd := &mockGit{rootDir: "/tmp"} + + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + assert.NotNil(t, templatizer) + assert.Equal(t, repoConfig, templatizer.repoConfig) + assert.Equal(t, gitcmd, templatizer.gitcmd) +} + +func TestFormatBodyStackOrder(t *testing.T) { + repoConfig := &config.RepoConfig{ + ShowPrTitlesInStack: false, + } + gitcmd := &mockGit{rootDir: "/tmp"} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + commit1 := git.Commit{CommitID: "commit1", Subject: "First"} + commit2 := git.Commit{CommitID: "commit2", Subject: "Second"} + commit3 := git.Commit{CommitID: "commit3", Subject: "Third"} + + commit := git.Commit{ + Subject: "Test", + Body: "Body text", + } + stack := []*github.PullRequest{ + {Number: 1, Commit: commit1}, + {Number: 2, Commit: commit2}, + {Number: 3, Commit: commit3}, + } + + result := templatizer.formatBody(commit, stack) + + // Stack should be in reverse order (3, 2, 1) + idx3 := strings.Index(result, "#3") + idx2 := strings.Index(result, "#2") + idx1 := strings.Index(result, "#1") + + assert.Greater(t, idx3, -1) + assert.Greater(t, idx2, -1) + assert.Greater(t, idx1, -1) + assert.True(t, idx3 < idx2, "#3 should come before #2") + assert.True(t, idx2 < idx1, "#2 should come before #1") +} + +func TestFormatBodyCurrentCommitIndicator(t *testing.T) { + repoConfig := &config.RepoConfig{ + ShowPrTitlesInStack: false, + } + gitcmd := &mockGit{rootDir: "/tmp"} + templatizer := NewCustomTemplatizer(repoConfig, gitcmd) + + commit1 := git.Commit{CommitID: "commit1", Subject: "First"} + commit2 := git.Commit{CommitID: "commit2", Subject: "Second"} + + commit := commit2 + stack := []*github.PullRequest{ + {Number: 1, Commit: commit1}, + {Number: 2, Commit: commit2}, + } + + result := templatizer.formatBody(commit, stack) + + // Current commit should have arrow indicator + assert.Contains(t, result, "#2 ⬅") + assert.NotContains(t, result, "#1 ⬅") +} diff --git a/github/template/template_stack/template.go b/github/template/template_stack/template.go new file mode 100644 index 00000000..74e1e700 --- /dev/null +++ b/github/template/template_stack/template.go @@ -0,0 +1,32 @@ +package template_stack + +import ( + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/ejoffe/spr/github/template" +) + +type StackTemplatizer struct { + showPrTitlesInStack bool +} + +func NewStackTemplatizer(showPrTitlesInStack bool) *StackTemplatizer { + return &StackTemplatizer{showPrTitlesInStack: showPrTitlesInStack} +} + +func (t *StackTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + return commit.Subject +} + +func (t *StackTemplatizer) Body(info *github.GitHubInfo, commit git.Commit) string { + body := commit.Body + + // Always show stack section and notice + body += "\n" + body += "---\n" + body += "**Stack**:\n" + body += template.FormatStackMarkdown(commit, info.PullRequests, t.showPrTitlesInStack) + body += "---\n" + body += template.ManualMergeNotice() + return body +} diff --git a/github/template/template_stack/template_test.go b/github/template/template_stack/template_test.go new file mode 100644 index 00000000..81588ab2 --- /dev/null +++ b/github/template/template_stack/template_test.go @@ -0,0 +1,858 @@ +package template_stack + +import ( + "strings" + "testing" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/stretchr/testify/assert" +) + +func TestTitle(t *testing.T) { + templatizer := NewStackTemplatizer(false) + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + want string + }{ + { + name: "simple subject", + commit: git.Commit{ + Subject: "Fix bug in authentication", + Body: "Some body text", + }, + want: "Fix bug in authentication", + }, + { + name: "empty subject", + commit: git.Commit{ + Subject: "", + Body: "Some body text", + }, + want: "", + }, + { + name: "subject with special characters", + commit: git.Commit{ + Subject: "Add feature: user authentication (WIP)", + Body: "Some body text", + }, + want: "Add feature: user authentication (WIP)", + }, + { + name: "subject with newline", + commit: git.Commit{ + Subject: "Fix bug\nwith newline", + Body: "Some body text", + }, + want: "Fix bug\nwith newline", + }, + { + name: "long subject", + commit: git.Commit{ + Subject: "This is a very long commit subject that might be used in some cases where developers write detailed commit messages", + Body: "Some body text", + }, + want: "This is a very long commit subject that might be used in some cases where developers write detailed commit messages", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Title(info, tt.commit) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestBody_EmptyStack(t *testing.T) { + templatizer := NewStackTemplatizer(false) + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{}, + } + + commit := git.Commit{ + Subject: "Test commit", + Body: "Commit body text", + } + + result := templatizer.Body(info, commit) + + // Should contain the commit body + assert.Contains(t, result, "Commit body text") + + // Should contain the manual merge notice + assert.Contains(t, result, "⚠️") + assert.Contains(t, result, "Part of a stack created by [spr]") + assert.Contains(t, result, "Do not merge manually using the UI") + + // Should end with the notice + assert.True(t, strings.HasSuffix(result, "Do not merge manually using the UI - doing so may have unexpected results.*")) +} + +func TestBody_WithStack_NoTitles(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First commit", + Body: "First body", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second commit", + Body: "Second body", + } + commit3 := git.Commit{ + CommitID: "commit3", + Subject: "Third commit", + Body: "Third body", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + { + Number: 1, + Title: "First commit", + Commit: commit1, + }, + { + Number: 2, + Title: "Second commit", + Commit: commit2, + }, + { + Number: 3, + Title: "Third commit", + Commit: commit3, + }, + }, + } + + // Test with commit2 (middle of stack) + result := templatizer.Body(info, commit2) + + // Should contain the commit body + assert.Contains(t, result, "Second body") + + // Should contain stack markdown (in reverse order: 3, 2, 1) + // Stack is formatted in reverse, so commit3 should be first + assert.Contains(t, result, "#3") + assert.Contains(t, result, "#2") + assert.Contains(t, result, "#1") + + // Current commit (commit2) should have the arrow indicator + assert.Contains(t, result, "#2 ⬅") + + // Other commits should not have the arrow + // We need to check that #1 and #3 don't have the arrow + lines := strings.Split(result, "\n") + foundCurrent := false + for _, line := range lines { + if strings.Contains(line, "#2 ⬅") { + foundCurrent = true + } + // Other PR numbers should not have the arrow + if strings.Contains(line, "#1") && !strings.Contains(line, "#2") { + assert.NotContains(t, line, "⬅", "PR #1 should not have arrow indicator") + } + if strings.Contains(line, "#3") && !strings.Contains(line, "#2") { + assert.NotContains(t, line, "⬅", "PR #3 should not have arrow indicator") + } + } + assert.True(t, foundCurrent, "Should find current commit with arrow indicator") + + // Should contain the manual merge notice + assert.Contains(t, result, "⚠️") + assert.Contains(t, result, "Part of a stack created by [spr]") +} + +func TestBody_WithStack_WithTitles(t *testing.T) { + templatizer := NewStackTemplatizer(true) + + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First commit", + Body: "First body", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second commit", + Body: "Second body", + } + commit3 := git.Commit{ + CommitID: "commit3", + Subject: "Third commit", + Body: "Third body", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + { + Number: 1, + Title: "First commit", + Commit: commit1, + }, + { + Number: 2, + Title: "Second commit", + Commit: commit2, + }, + { + Number: 3, + Title: "Third commit", + Commit: commit3, + }, + }, + } + + // Test with commit2 (middle of stack) + result := templatizer.Body(info, commit2) + + // Should contain the commit body + assert.Contains(t, result, "Second body") + + // Should contain PR titles in the stack + assert.Contains(t, result, "First commit #1") + assert.Contains(t, result, "Second commit #2") + assert.Contains(t, result, "Third commit #3") + + // Current commit should have the arrow + assert.Contains(t, result, "Second commit #2 ⬅") + + // Should contain the manual merge notice + assert.Contains(t, result, "⚠️") +} + +func TestBody_StackOrder(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First", + Body: "Body 1", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second", + Body: "Body 2", + } + commit3 := git.Commit{ + CommitID: "commit3", + Subject: "Third", + Body: "Body 3", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: commit1}, + {Number: 2, Commit: commit2}, + {Number: 3, Commit: commit3}, + }, + } + + result := templatizer.Body(info, commit2) + + // Stack should be in reverse order (3, 2, 1) + // Find the stack section + stackStart := strings.Index(result, "- #") + assert.Greater(t, stackStart, -1, "Should find stack markdown") + + stackSection := (result)[stackStart:] + + // Check order: #3 should come before #2, #2 before #1 + idx3 := strings.Index(stackSection, "#3") + idx2 := strings.Index(stackSection, "#2") + idx1 := strings.Index(stackSection, "#1") + + assert.Greater(t, idx3, -1, "Should find #3") + assert.Greater(t, idx2, -1, "Should find #2") + assert.Greater(t, idx1, -1, "Should find #1") + + // Verify reverse order + assert.True(t, idx3 < idx2, "#3 should come before #2") + assert.True(t, idx2 < idx1, "#2 should come before #1") +} + +func TestBody_CurrentCommitAtStart(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First", + Body: "Body 1", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second", + Body: "Body 2", + } + commit3 := git.Commit{ + CommitID: "commit3", + Subject: "Third", + Body: "Body 3", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: commit1}, + {Number: 2, Commit: commit2}, + {Number: 3, Commit: commit3}, + }, + } + + // Test with commit3 (last in stack, first in reverse order) + result := templatizer.Body(info, commit3) + + // Should have arrow on #3 + assert.Contains(t, result, "#3 ⬅") + + // Should not have arrow on others + assert.NotContains(t, result, "#1 ⬅") + assert.NotContains(t, result, "#2 ⬅") +} + +func TestBody_CurrentCommitAtEnd(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First", + Body: "Body 1", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second", + Body: "Body 2", + } + commit3 := git.Commit{ + CommitID: "commit3", + Subject: "Third", + Body: "Body 3", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: commit1}, + {Number: 2, Commit: commit2}, + {Number: 3, Commit: commit3}, + }, + } + + // Test with commit1 (first in stack, last in reverse order) + result := templatizer.Body(info, commit1) + + // Should have arrow on #1 + assert.Contains(t, result, "#1 ⬅") + + // Should not have arrow on others + assert.NotContains(t, result, "#2 ⬅") + assert.NotContains(t, result, "#3 ⬅") +} + +func TestBody_EmptyBody(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit := git.Commit{ + CommitID: "commit1", + Subject: "Test", + Body: "", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: commit}, + }, + } + + result := templatizer.Body(info, commit) + + // Should still contain stack and notice + assert.Contains(t, result, "#1") + assert.Contains(t, result, "⚠️") + assert.Contains(t, result, "Part of a stack created by [spr]") +} + +func TestBody_SinglePRInStack(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit := git.Commit{ + CommitID: "commit1", + Subject: "Single commit", + Body: "Single body", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 42, Commit: commit}, + }, + } + + result := templatizer.Body(info, commit) + + // Should contain the body + assert.Contains(t, result, "Single body") + + // Should contain the PR number + assert.Contains(t, result, "#42") + + // Should have arrow on current commit + assert.Contains(t, result, "#42 ⬅") + + // Should contain the manual merge notice + assert.Contains(t, result, "⚠️") +} + +func TestBody_WithTitlesVsWithoutTitles(t *testing.T) { + commit1 := git.Commit{ + CommitID: "commit1", + Subject: "First", + Body: "Body 1", + } + commit2 := git.Commit{ + CommitID: "commit2", + Subject: "Second", + Body: "Body 2", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Title: "First PR", Commit: commit1}, + {Number: 2, Title: "Second PR", Commit: commit2}, + }, + } + + // Test without titles + templatizerNoTitles := NewStackTemplatizer(false) + resultNoTitles := templatizerNoTitles.Body(info, commit2) + + // Should NOT contain PR titles + assert.NotContains(t, resultNoTitles, "First PR") + assert.NotContains(t, resultNoTitles, "Second PR") + // Should only contain numbers + assert.Contains(t, resultNoTitles, "- #1") + assert.Contains(t, resultNoTitles, "- #2") + + // Test with titles + templatizerWithTitles := NewStackTemplatizer(true) + resultWithTitles := templatizerWithTitles.Body(info, commit2) + + // Should contain PR titles + assert.Contains(t, resultWithTitles, "First PR #1") + assert.Contains(t, resultWithTitles, "Second PR #2") +} + +func TestBody_Structure(t *testing.T) { + templatizer := NewStackTemplatizer(false) + + commit := git.Commit{ + CommitID: "commit1", + Subject: "Test", + Body: "Commit body content", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: commit}, + }, + } + + result := templatizer.Body(info, commit) + + // Verify structure: body + \n\n + stack + \n\n + notice + // The body should come first + assert.True(t, strings.HasPrefix(result, "Commit body content")) + + // Should have stack markdown + assert.Contains(t, result, "- #1") + + // Should end with manual merge notice + assert.True(t, strings.HasSuffix(result, "Do not merge manually using the UI - doing so may have unexpected results.*")) +} + +func TestBody_RealWorldExample(t *testing.T) { + templatizer := NewStackTemplatizer(true) + + commit1 := git.Commit{ + CommitID: "abc123", + Subject: "Add authentication middleware", + Body: "Implemented JWT token validation", + } + commit2 := git.Commit{ + CommitID: "def456", + Subject: "Add user login endpoint", + Body: "Created POST /login endpoint", + } + commit3 := git.Commit{ + CommitID: "ghi789", + Subject: "Add user registration", + Body: "Created POST /register endpoint", + } + + info := &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + { + Number: 10, + Title: "Add authentication middleware", + Commit: commit1, + }, + { + Number: 11, + Title: "Add user login endpoint", + Commit: commit2, + }, + { + Number: 12, + Title: "Add user registration", + Commit: commit3, + }, + }, + } + + // Test with middle commit + result := templatizer.Body(info, commit2) + + // Should contain commit body + assert.Contains(t, result, "Created POST /login endpoint") + + // Should contain all PRs in reverse order with titles + assert.Contains(t, result, "Add user registration #12") + assert.Contains(t, result, "Add user login endpoint #11 ⬅") + assert.Contains(t, result, "Add authentication middleware #10") + + // Should contain manual merge notice + assert.Contains(t, result, "⚠️") + assert.Contains(t, result, "Part of a stack created by [spr]") +} + +func TestNewStackTemplatizer(t *testing.T) { + // Test that constructor works correctly + templatizer1 := NewStackTemplatizer(false) + assert.NotNil(t, templatizer1) + + templatizer2 := NewStackTemplatizer(true) + assert.NotNil(t, templatizer2) + + // They should be different instances + assert.NotEqual(t, templatizer1, templatizer2) +} + +func TestFormatPullRequestBody(t *testing.T) { + simpleCommit := git.Commit{ + CommitID: "abc123", + CommitHash: "abcdef123456", + } + descriptiveCommit := git.Commit{ + CommitID: "def456", + CommitHash: "ghijkl7890", + Body: `This body describes my nice PR. +It even includes some **markdown** formatting.`} + + tests := []struct { + name string + commit git.Commit + info *github.GitHubInfo + expected string + }{ + { + name: "EmptyStack", + commit: git.Commit{}, + info: &github.GitHubInfo{ + PullRequests: []*github.PullRequest{}, + }, + expected: ` +--- +**Stack**: +--- +⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, + }, + { + name: "SinglePRStack", + commit: descriptiveCommit, + info: &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 2, Commit: descriptiveCommit}, + }, + }, + expected: `This body describes my nice PR. +It even includes some **markdown** formatting. +--- +**Stack**: +- #2 ⬅ +--- +⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, + }, + { + name: "TwoPRStack", + info: &github.GitHubInfo{ + PullRequests: []*github.PullRequest{ + {Number: 1, Commit: simpleCommit}, + {Number: 2, Commit: descriptiveCommit}, + }, + }, + expected: `This body describes my nice PR. +It even includes some **markdown** formatting. +--- +**Stack**: +- #2 ⬅ +- #1 +--- +⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, + commit: descriptiveCommit, + }, + } + + templatizer := NewStackTemplatizer(false) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + body := templatizer.Body(tc.info, tc.commit) + if body != tc.expected { + t.Fatalf("expected: '%v', actual: '%v'", tc.expected, body) + } + }) + } +} + +/* +func TestFormatPullRequestBody_ShowPrTitle(t *testing.T) { + simpleCommit := git.Commit{ + CommitID: "abc123", + CommitHash: "abcdef123456", + } + descriptiveCommit := git.Commit{ + CommitID: "def456", + CommitHash: "ghijkl7890", + Body: `This body describes my nice PR. +It even includes some **markdown** formatting.`} + + tests := []struct { + description string + commit git.Commit + stack []*github.PullRequest + }{ + { + description: "", + commit: git.Commit{}, + stack: []*github.PullRequest{}, + }, + { + description: `This body describes my nice PR. +It even includes some **markdown** formatting.`, + commit: descriptiveCommit, + stack: []*github.PullRequest{ + {Number: 2, Commit: descriptiveCommit}, + }, + }, + { + description: `This body describes my nice PR. +It even includes some **markdown** formatting. + +--- + +**Stack**: +- Title B #2 ⬅ +- Title A #1 + + +⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do not merge manually using the UI - doing so may have unexpected results.*`, + commit: descriptiveCommit, + stack: []*github.PullRequest{ + {Number: 1, Commit: simpleCommit, Title: "Title A"}, + {Number: 2, Commit: descriptiveCommit, Title: "Title B"}, + }, + }, + } + + for _, tc := range tests { + body := formatBody(tc.commit, tc.stack, true) + if body != tc.description { + t.Fatalf("expected: '%v', actual: '%v'", tc.description, body) + } + } +} + +func TestInsertBodyIntoPRTemplateHappyPath(t *testing.T) { + tests := []struct { + name string + body string + pullRequestTemplate string + repo *config.RepoConfig + pr *github.PullRequest + expected string + }{ + { + name: "create PR", + body: "inserted body", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "## Description", + PRTemplateInsertEnd: "## Checklist", + }, + pr: nil, + expected: ` +## Related Issues + + +## Description +inserted body + +## Checklist +- [ ] My code follows the style guidelines of this project`, + }, + { + name: "update PR", + body: "updated description", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "## Description", + PRTemplateInsertEnd: "## Checklist", + }, + pr: &github.PullRequest{ + Body: ` +## Related Issues +* Issue #1234 + +## Description +original description + +## Checklist +- [x] My code follows the style guidelines of this project`, + }, + expected: ` +## Related Issues +* Issue #1234 + +## Description +updated description + +## Checklist +- [x] My code follows the style guidelines of this project`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body, _ := insertBodyIntoPRTemplate(tt.body, tt.pullRequestTemplate, tt.repo, tt.pr) + if body != tt.expected { + t.Fatalf("expected: '%v', actual: '%v'", tt.expected, body) + } + }) + } +} + +func TestInsertBodyIntoPRTemplateErrors(t *testing.T) { + tests := []struct { + name string + body string + pullRequestTemplate string + repo *config.RepoConfig + pr *github.PullRequest + expected string + }{ + { + name: "no match insert start", + body: "inserted body", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "does not exist", + PRTemplateInsertEnd: "## Checklist", + }, + pr: nil, + expected: "no matches found: PR template insert start", + }, + { + name: "no match insert end", + body: "inserted body", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "## Description", + PRTemplateInsertEnd: "does not exist", + }, + pr: nil, + expected: "no matches found: PR template insert end", + }, + { + name: "multiple many matches insert start", + body: "inserted body", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "duplicate", + PRTemplateInsertEnd: "## Checklist", + }, + pr: nil, + expected: "multiple matches found: PR template insert start", + }, + { + name: "multiple many matches insert end", + body: "inserted body", + pullRequestTemplate: ` +## Related Issues + + +## Description + + +## Checklist +- [ ] My code follows the style guidelines of this project`, + repo: &config.RepoConfig{ + PRTemplateInsertStart: "## Description", + PRTemplateInsertEnd: "duplicate", + }, + pr: nil, + expected: "multiple matches found: PR template insert end", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := insertBodyIntoPRTemplate(tt.body, tt.pullRequestTemplate, tt.repo, tt.pr) + if !strings.Contains(err.Error(), tt.expected) { + t.Fatalf("expected: '%v', actual: '%v'", tt.expected, err.Error()) + } + }) + } +} +*/ diff --git a/github/template/template_why_what/template.go b/github/template/template_why_what/template.go new file mode 100644 index 00000000..e69e8bb1 --- /dev/null +++ b/github/template/template_why_what/template.go @@ -0,0 +1,142 @@ +package template_why_what + +import ( + "bytes" + "strings" + go_template "text/template" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/ejoffe/spr/github/template" +) + +type WhyWhatTemplatizer struct{} + +func NewWhyWhatTemplatizer() *WhyWhatTemplatizer { + return &WhyWhatTemplatizer{} +} + +func (t *WhyWhatTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + return commit.Subject +} + +func (t *WhyWhatTemplatizer) Body(info *github.GitHubInfo, commit git.Commit) string { + // Split commit body by empty lines and filter out empty sections + sections := splitByEmptyLines(commit.Body) + + // Extract sections: first = Why, second = WhatChanged, third = TestPlan + // Multiple newlines between sections are treated the same as single newline + var why, whatChanged, testPlan string + if len(sections) > 0 { + why = sections[0] + } + if len(sections) > 1 { + whatChanged = sections[1] + } + if len(sections) > 2 { + testPlan = sections[2] + } + + // Prepare template data + data := struct { + Why string + WhatChanged string + TestPlan string + }{ + Why: why, + WhatChanged: whatChanged, + TestPlan: testPlan, + } + + // Parse and execute template + tmpl, err := go_template.New("why_what").Parse(whyWhatTemplate) + if err != nil { + // If template parsing fails, return the original body + return commit.Body + } + + var buf bytes.Buffer + if err := tmpl.Execute(&buf, data); err != nil { + // If template execution fails, return the original body + return commit.Body + } + + body := buf.String() + + // Always show stack section and notice + body += "\n" + body += "---\n" + body += "**Stack**:\n" + body += template.FormatStackMarkdown(commit, info.PullRequests, true) + body += "---\n" + body += template.ManualMergeNotice() + return body +} + +// splitByEmptyLines splits a string by empty lines (one or more consecutive newlines) +// Multiple consecutive newlines are treated as a single separator +// Empty sections are filtered out - only non-empty sections are returned +func splitByEmptyLines(text string) []string { + if text == "" { + return []string{} + } + + // Split by double newline (handles multiple newlines as single separator) + parts := strings.Split(text, "\n\n") + sections := make([]string, 0) + + for _, part := range parts { + trimmed := strings.TrimSpace(part) + // Only include non-empty sections + if trimmed != "" { + sections = append(sections, trimmed) + } + } + + // If no sections found but text exists (single newline case), treat entire text as one section + if len(sections) == 0 && strings.TrimSpace(text) != "" { + sections = append(sections, strings.TrimSpace(text)) + } + + return sections +} + +const whyWhatTemplate = ` +Why +=== + +{{ if ne .Why "" }} +{{ .Why }} +{{ else }} + +{{ end }} + +What changed +============ + +{{ if ne .WhatChanged "" }} +{{ .WhatChanged }} +{{ else }} + +{{ end }} + +Test plan +========= + +{{ if ne .TestPlan "" }} +{{ .TestPlan }} +{{ else }} + +{{ end }} + +Rollout +======= + + + +- [x] This is fully backward and forward compatible +` diff --git a/github/template/template_why_what/template_test.go b/github/template/template_why_what/template_test.go new file mode 100644 index 00000000..a9dd41cd --- /dev/null +++ b/github/template/template_why_what/template_test.go @@ -0,0 +1,405 @@ +package template_why_what + +import ( + "strings" + "testing" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/github" + "github.com/stretchr/testify/assert" +) + +func TestTitle(t *testing.T) { + templatizer := &WhyWhatTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + want string + }{ + { + name: "simple subject", + commit: git.Commit{ + Subject: "Fix bug in authentication", + Body: "Some body text", + }, + want: "Fix bug in authentication", + }, + { + name: "empty subject", + commit: git.Commit{ + Subject: "", + Body: "Some body text", + }, + want: "", + }, + { + name: "subject with special characters", + commit: git.Commit{ + Subject: "Add feature: user authentication (WIP)", + Body: "Some body text", + }, + want: "Add feature: user authentication (WIP)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Title(info, tt.commit) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestBody(t *testing.T) { + templatizer := &WhyWhatTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + want string + contains []string // strings that should be in the output + notContains []string // strings that should NOT be in the output + }{ + { + name: "all three sections provided", + commit: git.Commit{ + Subject: "Test commit", + Body: "This is why we made the change\n\nThis is what changed\n\nThis is the test plan", + }, + contains: []string{ + "Why\n===", + "This is why we made the change", + "What changed\n============", + "This is what changed", + "Test plan\n=========", + "This is the test plan", + "Rollout\n=======", + "- [x] This is fully backward and forward compatible", + }, + notContains: []string{ + "Describe what prompted you to make this change", + "Describe what changed to a level of detail", + }, + }, + { + name: "only why section provided", + commit: git.Commit{ + Subject: "Test commit", + Body: "This is why we made the change", + }, + contains: []string{ + "Why\n===", + "This is why we made the change", + "What changed\n============", + "Describe what changed to a level of detail", + "Test plan\n=========", + "You must provide a test plan", + }, + }, + { + name: "why and what changed sections provided", + commit: git.Commit{ + Subject: "Test commit", + Body: "This is why we made the change\n\nThis is what changed", + }, + contains: []string{ + "Why\n===", + "This is why we made the change", + "What changed\n============", + "This is what changed", + "Test plan\n=========", + "You must provide a test plan", + }, + notContains: []string{ + "Describe what prompted you to make this change", + "Describe what changed to a level of detail", + }, + }, + { + name: "empty body", + commit: git.Commit{ + Subject: "Test commit", + Body: "", + }, + contains: []string{ + "Why\n===", + "Describe what prompted you to make this change", + "What changed\n============", + "Describe what changed to a level of detail", + "Test plan\n=========", + "You must provide a test plan", + }, + }, + { + name: "body with only whitespace", + commit: git.Commit{ + Subject: "Test commit", + Body: " \n\n \n ", + }, + contains: []string{ + "Why\n===", + "Describe what prompted you to make this change", + }, + }, + { + name: "sections with extra whitespace", + commit: git.Commit{ + Subject: "Test commit", + Body: " Why section with spaces \n\n What changed section \n\n Test plan section ", + }, + contains: []string{ + "Why section with spaces", + "What changed section", + "Test plan section", + }, + }, + { + name: "multiple empty lines between sections", + commit: git.Commit{ + Subject: "Test commit", + Body: "Why section\n\n\n\nWhat changed section\n\n\n\nTest plan section", + }, + contains: []string{ + "Why section", + "What changed section", + "Test plan section", + }, + }, + { + name: "sections with newlines within them", + commit: git.Commit{ + Subject: "Test commit", + Body: "Why section\nwith multiple\nlines\n\nWhat changed\nwith multiple\nlines\n\nTest plan\nwith multiple\nlines", + }, + contains: []string{ + "Why section\nwith multiple\nlines", + "What changed\nwith multiple\nlines", + "Test plan\nwith multiple\nlines", + }, + }, + { + name: "only second section (what changed) provided", + commit: git.Commit{ + Subject: "Test commit", + Body: "\n\nWhat changed section", + }, + contains: []string{ + "Why\n===", + "What changed section", // First non-empty section goes to Why + "What changed\n============", + "Describe what changed to a level of detail", // What Changed is empty, shows default + }, + }, + { + name: "only third section (test plan) provided", + commit: git.Commit{ + Subject: "Test commit", + Body: "\n\n\nTest plan section", + }, + contains: []string{ + "Why\n===", + "Test plan section", // First non-empty section goes to Why + "What changed\n============", + "Describe what changed to a level of detail", // What Changed is empty, shows default + "Test plan\n=========", + "You must provide a test plan", // Test Plan is empty, shows default + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := templatizer.Body(info, tt.commit) + + // Check that all required strings are present + for _, wantStr := range tt.contains { + assert.Contains(t, got, wantStr, "Expected output to contain: %s", wantStr) + } + + // Check that excluded strings are not present + for _, notWantStr := range tt.notContains { + assert.NotContains(t, got, notWantStr, "Expected output to NOT contain: %s", notWantStr) + } + + // Verify the structure is correct + assert.Contains(t, got, "Why\n===") + assert.Contains(t, got, "What changed\n============") + assert.Contains(t, got, "Test plan\n=========") + assert.Contains(t, got, "Rollout\n=======") + assert.Contains(t, got, "- [x] This is fully backward and forward compatible") + }) + } +} + +func TestSplitByEmptyLines(t *testing.T) { + tests := []struct { + name string + input string + expected []string + }{ + { + name: "empty string", + input: "", + expected: []string{}, + }, + { + name: "single section", + input: "Single section text", + expected: []string{"Single section text"}, + }, + { + name: "single section with whitespace", + input: " Single section text ", + expected: []string{"Single section text"}, + }, + { + name: "two sections", + input: "First section\n\nSecond section", + expected: []string{"First section", "Second section"}, + }, + { + name: "three sections", + input: "First section\n\nSecond section\n\nThird section", + expected: []string{"First section", "Second section", "Third section"}, + }, + { + name: "sections with leading/trailing whitespace", + input: " First section \n\n Second section \n\n Third section ", + expected: []string{"First section", "Second section", "Third section"}, + }, + { + name: "multiple empty lines between sections", + input: "First section\n\n\n\nSecond section", + expected: []string{"First section", "Second section"}, // Empty sections filtered out + }, + { + name: "sections with internal newlines", + input: "First section\nwith multiple\nlines\n\nSecond section\nwith multiple\nlines", + expected: []string{"First section\nwith multiple\nlines", "Second section\nwith multiple\nlines"}, + }, + { + name: "only whitespace", + input: " \n\n \n ", + expected: []string{}, // All empty sections filtered out + }, + { + name: "empty lines at start and end", + input: "\n\nFirst section\n\nSecond section\n\n", + expected: []string{"First section", "Second section"}, // Empty sections filtered out + }, + { + name: "single newline (not empty line)", + input: "First section\nSecond section", + expected: []string{"First section\nSecond section"}, + }, + { + name: "mixed single and double newlines", + input: "First section\nSecond line\n\nThird section", + expected: []string{"First section\nSecond line", "Third section"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := splitByEmptyLines(tt.input) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestBodyTemplateStructure(t *testing.T) { + // This test ensures the template always produces the correct structure + templatizer := &WhyWhatTemplatizer{} + info := &github.GitHubInfo{} + + commit := git.Commit{ + Subject: "Test", + Body: "Why section\n\nWhat changed section\n\nTest plan section", + } + + result := templatizer.Body(info, commit) + + // Verify sections appear in correct order + whyIndex := strings.Index(result, "Why\n===") + whatIndex := strings.Index(result, "What changed\n============") + testIndex := strings.Index(result, "Test plan\n=========") + rolloutIndex := strings.Index(result, "Rollout\n=======") + + assert.Greater(t, whyIndex, -1, "Why section should be present") + assert.Greater(t, whatIndex, whyIndex, "What changed should come after Why") + assert.Greater(t, testIndex, whatIndex, "Test plan should come after What changed") + assert.Greater(t, rolloutIndex, testIndex, "Rollout should come after Test plan") +} + +func TestBodyWithRealWorldExamples(t *testing.T) { + templatizer := &WhyWhatTemplatizer{} + info := &github.GitHubInfo{} + + tests := []struct { + name string + commit git.Commit + }{ + { + name: "real commit message format", + commit: git.Commit{ + Subject: "Add user authentication", + Body: `We need to add authentication to secure the API endpoints. + +Added JWT token validation middleware and user login endpoint. + +Test by: +1. Start the server +2. Try to access protected endpoint without token +3. Login to get token +4. Access protected endpoint with token`, + }, + }, + { + name: "commit with only why", + commit: git.Commit{ + Subject: "Fix critical bug", + Body: "Users reported that the app crashes when clicking the submit button.", + }, + }, + { + name: "commit with markdown-like content", + commit: git.Commit{ + Subject: "Update documentation", + Body: `Documentation was outdated. + +- Updated API docs +- Added examples +- Fixed typos + +Manual review of the docs.`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := templatizer.Body(info, tt.commit) + + // Should always contain the required sections + assert.Contains(t, result, "Why\n===") + assert.Contains(t, result, "What changed\n============") + assert.Contains(t, result, "Test plan\n=========") + assert.Contains(t, result, "Rollout\n=======") + + // Should not be empty + assert.NotEmpty(t, result) + + // Should contain at least some content from the commit body + if tt.commit.Body != "" { + // Extract first line of body (likely to be in Why section) + firstLine := strings.Split(tt.commit.Body, "\n")[0] + if firstLine != "" { + assert.Contains(t, result, firstLine) + } + } + }) + } +} diff --git a/readme.md b/readme.md index f469ba24..b05e5819 100644 --- a/readme.md +++ b/readme.md @@ -167,12 +167,12 @@ User specific configuration is saved to .spr.yml in the user home directory. | githubHost | str | github.com | github host, can be updated for github enterprise use case | | mergeMethod | str | rebase | merge method, valid values: [rebase, squash, merge] | | mergeQueue | bool | false | use GitHub merge queue to merge pull requests | -| prTemplatePath | str | | path to PR template (e.g. .github/PULL_REQUEST_TEMPLATE/pull_request_template.md) | -| prTemplateInsertStart | str | | text to search for in PR template that determines body insert start location | -| prTemplateInsertEnd | str | | text to search for in PR template that determines body insert end location | +| prTemplateType | str | stack | PR template type, valid values: [stack, basic, why_what, custom]. If prTemplatePath is provided, this is automatically set to "custom" | +| prTemplatePath | str | | path to PR template file (e.g. .github/PULL_REQUEST_TEMPLATE/pull_request_template.md). When provided, prTemplateType is automatically set to "custom" | +| prTemplateInsertStart | str | | text marker in PR template that determines where to insert commit body (used with custom template type) | +| prTemplateInsertEnd | str | | text marker in PR template that determines where to end commit body insertion (used with custom template type) | | mergeCheck | str | | enforce a pre-merge check using 'git spr check' | | forceFetchTags | bool | false | also fetch tags when running 'git spr update' | -| branchNameIncludeTarget | bool | false | include target branch name in pull request branch name | | showPrTitlesInStack | bool | false | show PR titles in stack description within pull request body | | branchPushIndividually | bool | false | push branches individually instead of atomically (only enable to avoid timeouts) | | defaultReviewers | list | | default reviewers to add to each pull request | diff --git a/spr/spr.go b/spr/spr.go index 6b20dc10..4742bf97 100644 --- a/spr/spr.go +++ b/spr/spr.go @@ -170,7 +170,7 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string for i := range githubInfo.PullRequests { fn := func(i int) { pr := githubInfo.PullRequests[i] - sd.github.UpdatePullRequest(ctx, sd.gitcmd, githubInfo.PullRequests, pr, pr.Commit, nil) + sd.github.UpdatePullRequest(ctx, sd.gitcmd, githubInfo, githubInfo.PullRequests, pr, pr.Commit, nil) wg.Done() } if sd.synchronized { @@ -246,7 +246,7 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string for i := range updateQueue { fn := func(i int) { pr := updateQueue[i] - sd.github.UpdatePullRequest(ctx, sd.gitcmd, sortedPullRequests, pr.pr, pr.commit, pr.prevCommit) + sd.github.UpdatePullRequest(ctx, sd.gitcmd, githubInfo, sortedPullRequests, pr.pr, pr.commit, pr.prevCommit) wg.Done() } if sd.synchronized { @@ -322,7 +322,7 @@ func (sd *stackediff) MergePullRequests(ctx context.Context, count *uint) { prToMerge := githubInfo.PullRequests[prIndex] // Update the base of the merging pr to target branch - sd.github.UpdatePullRequest(ctx, sd.gitcmd, githubInfo.PullRequests, prToMerge, prToMerge.Commit, nil) + sd.github.UpdatePullRequest(ctx, sd.gitcmd, githubInfo, githubInfo.PullRequests, prToMerge, prToMerge.Commit, nil) sd.profiletimer.Step("MergePullRequests::update pr base") // Merge pull request