Define pull request conventions #9
Comments
Please, please, omit that
I don't think conventions should care how user organises their local branches? Also, most likely external contributors won't follow branch naming conventions. Will you reject their PRs just because they named the branch unconventionally? Note that it isn't possible to change branch name without recreating PR. All the rest: I'm +1 about |
|
Thanks for that feedback, Marat!
As I stated, these are "meant to be general guidelines, not strict rules" and "we can't mandate behavior". This is about leading by example. So no, I would not consider the name of the branch to be grounds for rejecting a PR as long as it's a dedicated branch. However, if it's the main branch, that's when I would consider enforcing the rule and having the contributor resubmit.
I'm definitely willing to reconsider my practice here. However, I must profess that it's convenient to be able to jump directly from the history to the pull request, which GitHub otherwise doesn't provide. I suppose we could restate this to say that we should ensure the issue is linked to the PR in the GitHub interface. That way, it's just one more click away. |
I mean, GitHub provides a way to jump to PR, see asciidoctor/asciidoctor-epub3@38dd012 for example. So, commit message has both link to issue and link to PR. It just doesn't have |
|
And yep, I am literally talking about these three symbols: P, R and whitespace. |
Aha! I see what you're saying. That would be fine by me. |
|
👍 So, WRT branch naming - I'm negative-neutral about adding such rule. When you see someone else's branch, you see it as a PR. So it is too late to rename it. When you create your own branch - name it whatever you like. We use rebase'n'squash strategy, so merged branch name doesn't appear in git history of target repository. So, what does branch name affect? The text that you see in PRs ( |
|
It helps considerably when I add the person's remote to my own repository and I'm navigating between branches. Without the issue number in the branch name, it quickly becomes confusing which branch is which. And the issue number alone gives no context outside of the issue tracker. So having both is idea. I'm speaking here from several years of experience now using this practice. And it has proven itself to be very effective in my own workflow. Because it has made such a difference, I am now advocating for it as a best practice / guideline. |
|
Does '*merge strategy*' include the merge type? aka-. Merge commit, squash
& merge, rebase
…On Wed, Mar 31, 2021 at 10:18 PM Dan Allen ***@***.***> wrote:
It helps considerably when I add the person's remote to my own repository
and I'm navigating between branches. Without the issue number in the branch
name, it quickly becomes confusing which branch is which. And the issue
number alone gives no context outside of the issue tracker. So having both
is idea.
I'm speaking here from several years of experience now using this
practice. And it has proven itself to be very effective in my own workflow.
Because it has made such a difference, I am now advocating for it as a best
practice / guideline.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMDNIMZZWI5WIKSNM3RCJTTGN7STANCNFSM42DFOLSQ>
.
|
Yes, that's what I'm referring to. And I don't think I would suggest which one a project should use. But whatever one the project does use, I think how it's annotated should be consistent across projects. I tend to use the squash and merge unless the PR has multiple discrete stages, in which case I might use the merge commit. But to be honest, I'm never quite sure what the right decision is. |
|
Good to know
Personally I've been switching between squash and merge-commit, but for
sake of simplicity I lately lean heavily toward squash.
The 1 to 1 relationship between commit & PR helps my sanity.
regards,
…On Wed, Mar 31, 2021 at 11:08 PM Dan Allen ***@***.***> wrote:
Does '*merge strategy*' include the merge type? aka-. Merge commit, squash
& merge, rebase
Yes, that's what I'm referring to. And I don't think I would suggest which
one a project should use. But whatever one the project does use, I think
how it's annotated should be consistent across projects.
I tend to use the squash and merge unless the PR has multiple discrete
stages, in which case I might use the merge commit. But to be honest, I'm
never quite sure what the right decision is.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMDNIO33DX3IDTOKDWE6QLTGOFLNANCNFSM42DFOLSQ>
.
|
For reference, I'm occasionally using |
Within the Asciidoctor community of projects, I think we should agree on a general set of conventions / guidelines for PRs (at the very least, for the core projects and those closely associated with it). This will make it easier to work in the various projects without having to remember what all the individual rules are. I have been adhering certain conventions over the years, but even those may need to be revisited and revised. What I'm about to propose are meant to be general guidelines, not strict rules. But the closer we stick to the guidelines, the easier it is to get into the flow.
issue requirement - First, I think it should be a recommendation that every human-created PR has an associated issue. The idea is that the issue is where the decision is made to approve the proposed idea and proceed with a code fix before anyone has invested time working on it. It's also where the architecture for the issue can be discussed. The issue number is what gets referenced by the CHANGELOG, so we want that to point to that initial conversation / full context. The PR can then be focused on the code review itself.
branch name - Second, The branch from which the PR is submitted should include the issue number and a short description in the form issue-<issue number>-<desc> (e.g., issue-1897-keep-anchor-with-block). This makes it easier to organize branches locally and ensures we don't get PRs from branches that are changing while we are trying to review the PR.
subject line - Third, we should agree on the format for the subject line for a PR. Should it start with an action keyword, like fixes|resolves|closes <issue number>? If we use these prefixes, when it's merged, the issue automatically gets closed. I know that GitHub refuses to recognize the issue number in the subject. But if it's not in the subject, then you can't quickly scan for issue-closing commits in the git history after merging it. So perhaps we need it in both the subject and the description.
changelog entry - Fourth, every PR should include an addition to the CHANGELOG. I know some people like to generate a CHANGELOG at the time of a release, but then no one knows what's in a release until it's made. Keeping the CHANGELOG up to date with each merge has proved to be very effective in communicating what's coming. It's a living document. It also makes doing the release a lot simpler.
merge strategy - Finally, how do we merge and what template do we use for the subject? Thus far, I've been using
resolves #<issue number> summary goes here (PR #<pr number>). Does that seem right? Here's how it looks: asciidoctor/asciidoctor@a1505efOf course, we can't mandate behavior. That's not the point. The point is for us maintainers to lead by example and develop a workflow that's consistent and works well across the projects. What other recommendations would you add that have proven to work well?
The text was updated successfully, but these errors were encountered: