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

Don't use the PR body in the homu merge message #37

Closed
wants to merge 1 commit into from

Conversation

emilio
Copy link
Member

@emilio emilio commented May 12, 2016

This is not optimal, but worth discussing it.

Probably we could be smarter and use a regexp to keep just the Fixes|Closes|Adresses #xxx messages.

Fixes servo/servo#11153
Fixes #36


This change is Reviewable

This is not optimal, but worth discussing it.

Probably we could be smarter and use a regexp to keep just the `Fixes|Closes|Adresses #xxx` messages.

Fixes servo/servo#11153
Fixes #36
@emilio
Copy link
Member Author

emilio commented May 12, 2016

Since stripping the reviewable thing is easy, we could also do a servo-specific thing and look for the Thanks for contributing to Servo message, and strip from there.

But that would assume the proper PR description is before that.

@cgwalters
Copy link

Do we need to keep Closes: from the original commit? Won't adding a commit with it to master still close the issue?

This patch seems OK to me, but all my uses of Homu are linear = True.

@emilio
Copy link
Member Author

emilio commented May 12, 2016

Yes, if it's in the commit description it will still close the issue, but some contributors add the Closes: xxx only to the PR description, and I'm not totally sure if Github would close the issue in that case (probably not).

@Manishearth
Copy link
Member

For Rust I find this info super useful since I don't have to leave my terminal and can go through merge commits very fast. I've had to do this less in Servo (changes are more localized and easier to spot), but it's still useful.

IMO stripping reviewable info might be better. Or make it configurable.

@Manishearth
Copy link
Member

Also, yeah, github doesn't close the issue otherwise

@nox
Copy link

nox commented May 16, 2016

The greetings at the very least should be removed from the commit message, IMO.

@edunham
Copy link

edunham commented Mar 13, 2017

@emilio, thanks for starting the discussion on this! It looks to me like the discussion here continued into #40 and reached a resolution that we should use custom tags within the PR template to tell Homu what content to leave out of the merge commit. It looks to me like this should be closed in favor of fixups on #40, but if I missed something in drawing that conclusion, you're more than welcome to reopen.

@edunham edunham closed this Mar 13, 2017
@emilio
Copy link
Member Author

emilio commented Mar 13, 2017

Nope, thanks for closing this @edunham! :)

@emilio emilio deleted the emilio-patch-1 branch March 13, 2017 22:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants