gromero commented on PR #88: URL: https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1215988221
> Can you also include guidelines for 1. how the PR author can update their commits if they are not up to this standard (rebase?) Let's say a PR has just been created. The idea is that regarding the commit message (that will compose the final commit message) it doesn't matter the commit(s) message(s) of the commits used to create the PR. It really only matters the PR's title and body. So rather than a rebase + push force it's just a matter of updating the PR's title and body in the GH GUI. Now, I believe you are thinking of the case when the author's commits are a rather difficult (like, quite badly organized) set of patches and the PR's title and body also don't help to understand the change. Since it's the "initial" commits (the PR has just been created), I think that it would be awesome that the author would reorganize it, splitting the commits in a reasonable way, then rebase and push again it, updating the PR title and body (or even creating a brand new PR) that would ease the review. That's what I tried to capture here in this part: https://github.com/apache/tvm-rfcs/pull/88/commits/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R126 but merely **a recommendation**, moreover now we have to consider also the comment from Matthew about multiple in a PR: https://github.com/apache/tvm-rfcs/pull/88#discussion_r946033889 Personally, I don't want people to stuff a bunch of non-related commits into a PR, but, on the other hand I also think it helps me review the changes when the commits are split in reasonable way -- just for a single self-contained feature / change -- even if they will be squashed before merge (which is a pity I might say). But capturing such a nuances in the guideline in very case is hard. > 2. How reviewers should handle squashing an merging (should they copy text from the PR description or from a commit) I tried to address it right in the beginning here: https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R46-R50 and then in the end here: https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R138-R139 i.e. they should copy text from the PR description, title and body. >3. how to handle commits that fix linting or formatting issues in the original PR. Do you mean the additional commits that are pushed to the branch as a consequence of the review process right? The ones we've been calling "additional commit messages"? If so, here is where I tried to address it: https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R134-R135 i.e. it doesn't matter. All that matters for the final commit message that lands into the main tree is to keep the PR's title and body updated and following the Commit Message Guideline. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
