Skip to content
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

Tidying commit messages #16009

Closed
wafflespeanut opened this issue Mar 17, 2017 · 5 comments
Closed

Tidying commit messages #16009

wafflespeanut opened this issue Mar 17, 2017 · 5 comments
Labels

Comments

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Mar 17, 2017

I got to see this post today. It reminded me that most often, we don't really worry about the commit messages (we just stamp r+ and go). Given our rules for clean code, I think we could have some basic rules for commit messages as well. As of now, this is all I can think of:

  • Commit messages should begin with a title case.
  • The first line of messages shouldn't be longer than 80 characters (other details can go below in a separate paragraph) - for example, this is useful and neat, whereas this is long, and it's been trimmed, and is not very pleasing.
  • Not sure about trailing punctuation (I suppose we shouldn't allow it for the first line?)
  • Links shouldn't be allowed in the first line (given the limit of 80 chars). It could be moved to the details below.

Shall we make tidy check all this?

@wafflespeanut wafflespeanut added the B-RFC label Mar 17, 2017
@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Mar 17, 2017

One cool thing that projects under the JQuery foundation have is to include a word or a small phrase in the beginning of the commit message which states what part of the repo is being affected by the commit. This can be followed by a short, one line description. For example:

Event: Add radio click triggering tests

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Mar 17, 2017

I think it’s good to have guidelines for commit messages. I think it’s good to have tidy check for code in the repository, since that code can stay for a long time. I don’t think gating pull requests (or worse, try builds) on details of commit message formatting or punctuation is that useful, it sounds very annoying for little benefit. It’s better to live with improper punctuation in commit message than to force a PR to go through another cycle of review and CI.

@wafflespeanut
Copy link
Member Author

@wafflespeanut wafflespeanut commented Mar 17, 2017

That's one other thing I wanted to ask, whether we should do this in homu, or whether we can keep it to Travis and Appveyor builds? (by having a tidy argument)

@notriddle
Copy link
Contributor

@notriddle notriddle commented Mar 25, 2017

This is exactly what GitCop is for. It would run before the commit even gets to bors (though bors should probably check that GitCop has successfully passed before putting the PR onto the queue).

@wafflespeanut
Copy link
Member Author

@wafflespeanut wafflespeanut commented Jun 3, 2017

Discussion about GitCop in #16138 and the dev-servo mailing list. Closing this in favor of that.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.