doc: additional guidance about amending commits - #41287
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
In my first contribution, I got the amending guidance wrongly and amended my commit to attend some requested changes. So I added some more lines in the docs to help the further contributors to don't make the same mistake that I did
|
I suggest instead we should remove this paragraph: node/doc/guides/contributing/pull-requests.md Lines 312 to 319 in e676942 I think we don't want contributors to amend already-pushed commits in general, the only use case I can think of is if we want the commit message to change, but for most PRs that just doesn't happen (and if it does happen and the contributor doesn't know how to do it, a Node.js collaborator can do it for them). I'm thinking of something like https://github.com/nodejs/node/pull/41299/files#diff-b989d7ccc0bd9a756a048f0255df3bd9444a63f8cb667e4b7b2a9575da1800a0R307-R311. wdyt? |
Sorry, something went wrong.
|
@aduh95 There's a pipeline that checks the first commit message, and when the PR has only one commit is easier to fix the commit message using amend, maybe the recommendation should be:
|
Sorry, something went wrong.
|
Note that if this check doesn't pass, it doesn't block the PR from landing. I don't think we should recommend folks from amending the commit; folks reading this guide are more likely to have checked the commit guidelines anyway, so chances are they won't need to amend their commit message. If the commit message is invalid and they notice the failed check once they open the PR, either they know already how to amend the commit message, either they don't, both are fine. |
Sorry, something went wrong.
|
@aduh95 that seems fair. I'll update the PR removing this part |
Sorry, something went wrong.
aduh95
left a comment
There was a problem hiding this comment.
LGTM. Do you want to amend the previous paragraph saying something like Force-pushing also complicates the review process, as it won't allow reviewers to get a quick glance on what have changed?
Sorry, something went wrong.
Making all drawbacks to be listed in a row Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
This way all the drawbacks are in a row
Fixing duplicated additional guidance about for pushing Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
In my first contribution, I got the amending guidance wrongly and amended my commit to attend some requested changes. Amending commits is never required to author a PR in the project, and force pushing makes reviewing harder, so the PR guide should not recommend it as a good practice. PR-URL: #41287 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
In my first contribution, I got the amending guidance wrongly and amended my commit to attend some requested changes. Amending commits is never required to author a PR in the project, and force pushing makes reviewing harder, so the PR guide should not recommend it as a good practice. PR-URL: #41287 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
In my first contribution, I got the amending guidance wrongly and amended my commit to attend some requested changes. Amending commits is never required to author a PR in the project, and force pushing makes reviewing harder, so the PR guide should not recommend it as a good practice. PR-URL: #41287 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
In my first contribution, I got the amending guidance
wrongly and amended my commit to attend some requested
changes. So I added some more lines in the docs to help the
further contributors to don't make the same mistake that I did