-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve PR info in default merge message #9635
Conversation
4d96381
to
0cd12d1
Compare
@SD1998 think you took the wrong issue number |
Codecov Report
@@ Coverage Diff @@
## master #9635 +/- ##
=========================================
Coverage ? 42.22%
=========================================
Files ? 582
Lines ? 77109
Branches ? 0
=========================================
Hits ? 32559
Misses ? 40552
Partials ? 3998
Continue to review full report at Codecov.
|
models/pull.go
Outdated
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err) | ||
return "" | ||
} | ||
return fmt.Sprintf("Merge pull request %d: '%s' from '%s' of %s/%s into %s", pr.IssueID, pr.Issue.Title, pr.HeadBranch, pr.MustHeadUserName(), pr.HeadRepo.Name, pr.BaseBranch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Sprintf("Merge pull request %d: '%s' from '%s' of %s/%s into %s", pr.IssueID, pr.Issue.Title, pr.HeadBranch, pr.MustHeadUserName(), pr.HeadRepo.Name, pr.BaseBranch) | |
return fmt.Sprintf("Merge pull request '%s' (#%d) from %s/%s into %s", pr.Issue.Title, pr.Issue.Index, pr.MustHeadUserName(), pr.HeadBranch, pr.BaseBranch) |
this is more like github + has still the PR title in it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need index not ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more 👀 see more - added to suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@6543 I have made the change.
…ll request details
0cd12d1
to
754d12e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but can it be merged for v1.12?
@bagasme yes, it will. |
This comment has been minimized.
This comment has been minimized.
Make lg-tm work |
@SD1998 @zeripath this PR does not fully satisfy ticket #9552, as it only changes the merge title, and does not put the PR description into the merge commit message body. Hence part of the issue remains outstanding. However, my question is how to modify this without altering the source code and recompiling? I preferred the old format, before this change. Many people will prefer the new format. It seems to be possible to put templates into each repository, to override - but is there a way to apply a system-wide default? If so, this would allow the both my question and the remaining parts of the issue to be addressed. Apologies if this is covered somewhere in documentation, but I could not find anything. |
Fix for issue #9552.