Skip to content
This repository was archived by the owner on Mar 20, 2024. It is now read-only.

Fix PR template and Reviewable from merge commits #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aneeshusa
Copy link

@aneeshusa aneeshusa commented May 27, 2016

We recently added a PR template to the main Servo repo to smooth the
contributing workflow. However, this template gets copied into homu's
merge commit messages, generating a lot of noise in the commit logs.

Use heuristics to strip both the PR template and the Reviewable footer.
First, the main body of the PR template and the Reviewable footer are
removed, as these are always the same (except for if the boxes are
checked) and always at the bottom. The other part of the PR template is
a header comment; some users replace it, while others leave it, and even
though the message says to write below that line, some users write above
it. So, simply remove it and trim any extra newlines. These heuristics
should robustly leave the body untouched if the template is changed.

Tested against an excerpt of recent commit messages from Servo.

Fixes #36. Fixes servo/servo#11153. Supersedes #37.


This change is Reviewable

We recently added a PR template to the main Servo repo to smooth the
contributing workflow. However, this template gets copied into homu's
merge commit messages, generating a lot of noise in the commit logs.

Use heuristics to strip both the PR template and the Reviewable footer.
First, the main body of the PR template and the Reviewable footer are
removed, as these are always the same (except for if the boxes are
checked) and always at the bottom. The other part of the PR template is
a header comment; some users replace it, while others leave it, and even
though the message says to write below that line, some users write above
it. So, simply remove it and trim any extra newlines. These heuristics
should robustly leave the body untouched if the template is changed.
This leaves a much cleaner commit message in the merge commit.

Tested against an excerpt of recent commit messages from Servo.
@aneeshusa aneeshusa force-pushed the strip-pr-template-from-merges branch from 5688e30 to 1189a45 Compare May 27, 2016 02:21
@Manishearth
Copy link
Member

So the checks are preserved? (since one of the rows asks you to mention an issue number, which we want)

@aneeshusa
Copy link
Author

Right now it strips that out, but I can make it keep that line if the issue number isn't blank. Is there anything else that should be kept from the PR template?

@Manishearth
Copy link
Member

Yeah, please keep it. Otherwise, not that I know of

@Ms2ger
Copy link

Ms2ger commented May 27, 2016

Can we add <!-- homu strip --> comments to the template instead?

@jdm
Copy link
Member

jdm commented May 27, 2016

Designating the parts to strip in the template itself makes sense to me. We should be sure to do so in a way that doesn't make it more confusing, however.

@cgwalters
Copy link

Yes, <!-- homu strip --> sounds a lot better to me - it avoids us hardcoding servo inside homu.

@nox
Copy link

nox commented Mar 2, 2017

Or <hr class="homu-strip">, this way it's even visible.

@bors-servo
Copy link

☔ The latest upstream changes (presumably #103) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants