Skip to content

Add a status check for reverted commits #72

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

Closed
berkerpeksag opened this issue Apr 13, 2017 · 10 comments
Closed

Add a status check for reverted commits #72

berkerpeksag opened this issue Apr 13, 2017 · 10 comments

Comments

@berkerpeksag
Copy link
Member

Currently, it's hard to know why a commit has been reverted. I wonder if we should require to have a "Reason:" field in commit messages.

bedevere seems like a good tool to implement this.

@Mariatta
Copy link
Member

Maybe we can start by mentioning this in the devguide 🤔

Mariatta added a commit to Mariatta/bedevere that referenced this issue Apr 14, 2017
If a PR starts with the word "Revert", then check if the
body contains the reason for the revert.

Closes python/core-workflow#72
@brettcannon
Copy link
Member

You might want to talk to python-committers or python-dev about this before implementing it as I have not heard this issue brought up prior to this issue.

@brettcannon
Copy link
Member

I should also mention core-workflow is also an acceptable place to ask about this. My point is that since this will be front and centre for everyone it should be discussed at least a little bit before it gets turned on.

@berkerpeksag
Copy link
Member Author

I don't really have time and motivation to start an endless thread on a mailing list. This was briefly mentioned by other core developers (I think the last time I read something about this was written by @terryjreedy) on various places over the past few years, but unfortunately I don't have a link.

@Mariatta
Copy link
Member

This is the most recent message about revert commit message in python-committers:

https://mail.python.org/pipermail/python-committers/2017-April/004389.html

@brettcannon
Copy link
Member

I agree that people should be filling in a message, I just don't know if requiring a "Reason:" prefix is necessary instead of simply telling people they are expected to give a reason. It's one of those questions of whether we need an explicit check or people just need to pick up the habit?

@terryjreedy
Copy link
Member

Although I clicked the 'like' button for the message that said 'require Reason: field', I am inclined to what Brett said. I would prefer a first line of 'Revert xyz because it broke Window buildbot' to having the reason buried in the details, if any. I also think we should carefully pick what is mechanically required.

@Mariatta
Copy link
Member

I'm good with just asking people to give a reason, and not needing a status check for it :)

@brettcannon
Copy link
Member

OK, with the update @Mariatta did to the devguide for providing guidance to have people say why something was reverted I think this can probably be closed. OK with you @berkerpeksag ?

@berkerpeksag
Copy link
Member Author

SGTM, thanks @Mariatta!

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

No branches or pull requests

4 participants