mumble
mumble copied to clipboard
DOCS(git): Add PR and merge commit guidance to commit guidelines
Implements #6385
Our scripts/generate_changelog.py
script pr_number_pattern
already supports PR numbers in the form (#<nr>)
. 1
The GitHub repository can be configured to merge with PR title + (#<nr>) + desc
by default, which would follow these suggested rules.
Our
scripts/generate_changelog.py
scriptpr_number_pattern
already supports PR numbers in the form(#<nr>)
. 1
It looks like it does, but it does not. The problem was that the regex match groups are enumerated based on the original regex string and not the number of groups actually matched. Therefore the script fails. I have fixed this before when creating the last RC, but I sadly somehow dropped the Mumble repo folder this change was in...
I will fix this again, when I draft the next RC
Our
scripts/generate_changelog.py
scriptpr_number_pattern
already supports PR numbers in the form(#<nr>)
. 1It looks like it does, but it does not. The problem was that the regex match groups are enumerated based on the original regex string and not the number of groups actually matched. Therefore the script fails. I have fixed this before when creating the last RC, but I sadly somehow dropped the Mumble repo folder this change was in...
I will fix this again, when I draft the next RC
I don't quite follow. Does this PR change influence whether or in what way the script breaks or fails? Or is the script broken after the same way it was broken before? Within the (broken) script context, does the regex not match PRs of the specified form? (Is this PR sound within its context?)
I don't quite follow. Does this PR change influence whether or in what way the script breaks or fails? Or is the script broken after the same way it was broken before? Within the (broken) script context, does the regex not match PRs of the specified form? (Is this PR sound within its context?)
The script is and was broken independent of this PR. It fails in subtle ways, that will become apparent once I post a PR to fix it. So don't worry about it for now. Just note that Our scripts/generate_changelog.py script pr_number_pattern already supports PR numbers in the form (#<nr>)
is only true in theory :)
@Krzmbrzl @davidebeatrici Are you fine with this change?
Merge commits would no longer be Merge PR #123: TYPE(scope): Title
but TYPE(scope): Title (#123)
.
Once approved and merged, I would/will change the repo setting so that is the default (no need for manual title and desc changes on merge).
Sounds good to me, consistency in PR merge titles was broken a while ago anyway.
On a separate note: the Rebase and merge
method should be considered, but in our case it may be useful to bring the PR's title and message into the commit history (i.e. keep the merge commit behavior).
I would prefer having an explicit Merge
appear in merge commits as they behave very differently during e.g. rebasing and I would like to know if I'll get issues while rebasing by a quick glance at git log
.
I would prefer having an explicit
Merge
appear in merge commits as they behave very differently during e.g. rebasing and I would like to know if I'll get issues while rebasing by a quick glance atgit log
.
I don't see a good solution for that. If we want the merge commits to follow commit format we either
- use PR titles that follows commit format, use the corresponding default, but have to manually add "merge" (error-prone)
- use PR titles that follows commit format, require adding "merge" to the PR title, use the corresponding default
- use the traditional default, require adjusting merge commit title (error-prone)
What's your concerns on rebase or otherwise? Rebase at least can preserve merge commits. (Which is not the default (it's omission) and not necessarily obvious or convenient.)
What's your concerns on rebase or otherwise?
Counting commits doesn't work across merge commits (for e.g. git rebase HEAD^n
)
Given there's been no solution suggestion to Krzmbrzls concern nor discussion I'll close this as unresolved/unresolvable.
I guess we'll continue with no guideline or documented intention then.