-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
DOCS(git): Add PR and merge commit guidance to commit guidelines #6391
base: master
Are you sure you want to change the base?
Conversation
Implements mumble-voip#6385 Our `scripts/generate_changelog.py` script `pr_number_pattern` already supports PR numbers in the form `(#<nr>)`. The GitHub repository can be configured to merge with PR title + (#<nr>) + desc, which follows this form. [1]: https://github.com/mumble-voip/mumble/blob/56f03e8d7e5f9cf9d1a318d3a1858db4e09c06ab/scripts/generate_changelog.py#L22-L24
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 |
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.
As I said I do not mind what kind of message merge commits get either way, so this gets a "neutral" approval from me 😄
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 |
@Krzmbrzl @davidebeatrici Are you fine with this change? Merge commits would no longer be 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 |
I would prefer having an explicit |
I don't see a good solution for that. If we want the merge commits to follow commit format we either
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.) |
Counting commits doesn't work across merge commits (for e.g. |
Implements #6385
Our
scripts/generate_changelog.py
scriptpr_number_pattern
already supports PR numbers in the form(#<nr>)
. 1The GitHub repository can be configured to merge with PR
title + (#<nr>) + desc
by default, which would follow these suggested rules.