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

doc: additional guidance about amending commits #41287

Merged
merged 6 commits into from
Dec 26, 2021

Conversation

Farenheith
Copy link
Contributor

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 22, 2021
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
@Farenheith Farenheith changed the title docs: additional guidance about amending commits doc: additional guidance about amending commits Dec 22, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

I suggest instead we should remove this paragraph:

If you happen to make a mistake in any of your commits, do not worry. You can
amend the last commit (for example if you want to change the commit log).
```text
$ git add any/changed/files
$ git commit --amend
$ git push --force-with-lease origin my-branch
```

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?

@Farenheith
Copy link
Contributor Author

Farenheith commented Dec 23, 2021

@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:

  • To keep one commit before the PR
  • To amend it in case of something wrong in the description. The fact that the pipeline that checks it fails fast helps with that.
  • To add new commits for every fix after the PR is opened

@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

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.

@Farenheith
Copy link
Contributor Author

Farenheith commented Dec 23, 2021

@aduh95 that seems fair. I'll update the PR removing this part

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Farenheith
Copy link
Contributor Author

@aduh95 done

Farenheith and others added 2 commits December 23, 2021 16:08
Making all drawbacks to be listed in a row

Co-authored-by: Antoine du Hamel <[email protected]>
This way all the drawbacks are in a row
Fixing duplicated additional guidance about for pushing

Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 23, 2021
@aduh95 aduh95 merged commit 94c9f62 into nodejs:master Dec 26, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 26, 2021

Landed in 94c9f62

targos pushed a commit that referenced this pull request Jan 14, 2022
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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: nodejs#41287
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants