[quote="driazati, post:5, topic:12334, full:true"] These guidelines are definitely good to have and I think we should codify them in our docs! One big problem we have today is clicking the merge button on GitHub defaults to a bad commit message which [[RFC] Allow merging via PR comments](https://discuss.tvm.apache.org/t/rfc-allow-merging-via-pr-comments/12220) should fix but would also be a workflow change. [/quote]
@driazati Thanks for pointing that RFC you posted. I totally missed it. Pretty good discussion in there. Yeah I'm aware about the merge button on GH being heavily guilty for a couple of issues associated with the bad commit messages, like title concatenation, etc. [quote] We can enforce some rules via automation, but getting people to change their behavior regarding these kind things is tricky. [/quote] Right, it's tricky. So although GH can be blamed for many aspects of the bad commit messages currently in the tree, there is a human/behavioral aspect that I firmly believe that needs to be improved and, above all, _can_ be improved :) I thought about this guideline as a first step for improving that aspect. That's why I also said that I would at first "left the items regarding Github-specific issues out of this proposal", because I wanted to focus on that tricky aspect in the end. In that sense I'm also happy that your RFC is addressing other GH and automation aspects more closely. [quote] [quote="gromero, post:1, topic:12334"] > * Use of imperative mood > * Proper use of caps at the beginning (uppercase for the first letter) > * No period at the end [/quote] With prescriptive rules like this they’re not really enforceable. For one lots of reviewers won’t follow a workflow outside their usual one (i.e. reviewing for commit language), and a review comment can be pretty costly, especially if the reviewer/author aren’t in the same time zone. Is it really worth it to incur this delay for something simple like a change in punctuation in the PR title? Alternatively, the onus could be entirely on the reviewer to edit the message as necessary to conform to the guidelines (but this makes reviewing harder). I think the ones we can do automatically via the mergebot RFC is ensure: * there is at least one ‘tag’ in the title (`[ci]`, `microtvm`, etc) * that the body is not empty [/quote] I see. Thanks for letting me know what we can achieve (possibly) by automation here, I'm aware you're working hard on the front. That's really good. I agree that _enforcing_ these rules might be hard, so maybe we should put these 3 rules (only?) as recommendations? Now, I do think that the reviewers/committers/maintainers must be aware of the code/commit guidelines and try to follow them at least when _they_ are submitting code, and use the guidelines with parsimony when reviewing code submitted by other authors/new contributors. For instance, the code will need to get fixed anyways, so another CI test/review round is unavoidable, hence just let the user know that the title can be improved to match the guidelines on the next version to be pushed. That's a quite simple ask that can help a lot a behavioral change. I think the onus should _not_ be on the reviewers, and I think that an ask like that would avoid it: it's a submitter's duty to write a good commit message, just like a correct/good code. [quote] [quote="tkonolige, post:3, topic:12334"] What are the guidelines for messages in commits that come after the initial/main ones? For example, commits to fix lint issues. [/quote] The commits on PRs should not have any bearing on the merged commit message (so no rules apply). They can be helpful for review, but the final message should be based on the PR title + PR body since that is where people usually consolidate the actual information (and it’s what GitHub surfaces by default, whereas you have to go digging for commits). [/quote] Yeah I agree. When I replied to Tristan above I kind was saying that there would be no way to address that issue so went for a few suggestions, but then I was not aware of your RFC that will (hopefully!) address it. In that case, I would leave any recommendation about the additional commit out of this guideline @tkonolige @driazati In that case are you considering that the submitter will be able to change the initial/original PR body, using a `git push --force` for instance, after an amend? --- [Visit Topic](https://discuss.tvm.apache.org/t/commit-message-guideline/12334/8) to respond. You are receiving this because you enabled mailing list mode. To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/29f7e92124eaeba250c4bd87346a6c4840b362d83d19d6d78e2cead70e2049b7).
