doc: standardize on _pull request_
Sometimes we capitalize _pull request_ and sometimes we don't. Standardize on lowercase based on Microsoft Style Guide, Chicago Manual of Style, and GitHub's own docs and UI. Refs: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests Refs: https://docs.github.com/en/github/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/merging-a-pull-request Refs: https://docs.microsoft.com/en-us/style-guide/capitalization PR-URL: https://github.com/nodejs/node/pull/39384 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Harshitha K P <harshitha014@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
This commit is contained in:
parent
575266ac1b
commit
7dbc0f74d0
@ -305,7 +305,7 @@ $ make test-only
|
|||||||
|
|
||||||
At this point, you are ready to make code changes and re-run the tests.
|
At this point, you are ready to make code changes and re-run the tests.
|
||||||
|
|
||||||
If you are running tests before submitting a Pull Request, the recommended
|
If you are running tests before submitting a pull request, the recommended
|
||||||
command is:
|
command is:
|
||||||
|
|
||||||
```console
|
```console
|
||||||
|
@ -488,7 +488,7 @@ The TSC serves as the final arbiter where required.
|
|||||||
who wish to land their own pull requests will self-assign them. Sometimes, an
|
who wish to land their own pull requests will self-assign them. Sometimes, an
|
||||||
author will delegate to someone else. If in doubt, ask the assignee whether
|
author will delegate to someone else. If in doubt, ask the assignee whether
|
||||||
it is okay to land.
|
it is okay to land.
|
||||||
1. Never use GitHub's green ["Merge Pull Request"][] button. Reasons for not
|
1. Never use GitHub's green ["Merge pull request"][] button. Reasons for not
|
||||||
using the web interface button:
|
using the web interface button:
|
||||||
* The "Create a merge commit" method will add an unnecessary merge commit.
|
* The "Create a merge commit" method will add an unnecessary merge commit.
|
||||||
* The "Squash and merge" method will add metadata (the pull request #) to the
|
* The "Squash and merge" method will add metadata (the pull request #) to the
|
||||||
@ -911,7 +911,7 @@ need to be attached anymore, as only important bugfixes will be included.
|
|||||||
* `arm`, `mips`, `s390`, `ppc`
|
* `arm`, `mips`, `s390`, `ppc`
|
||||||
* No `x86{_64}` label because it is the implied default
|
* No `x86{_64}` label because it is the implied default
|
||||||
|
|
||||||
["Merge Pull Request"]: https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github
|
["Merge pull request"]: https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github
|
||||||
[Deprecation]: https://en.wikipedia.org/wiki/Deprecation
|
[Deprecation]: https://en.wikipedia.org/wiki/Deprecation
|
||||||
[SECURITY.md]: https://github.com/nodejs/node/blob/HEAD/SECURITY.md
|
[SECURITY.md]: https://github.com/nodejs/node/blob/HEAD/SECURITY.md
|
||||||
[Stability Index]: ../api/documentation.md#stability-index
|
[Stability Index]: ../api/documentation.md#stability-index
|
||||||
|
@ -2,12 +2,12 @@
|
|||||||
|
|
||||||
> Stability: 1 - Experimental
|
> Stability: 1 - Experimental
|
||||||
|
|
||||||
*tl;dr: You can land Pull Requests by adding the `commit-queue` label to it.*
|
*tl;dr: You can land pull requests by adding the `commit-queue` label to it.*
|
||||||
|
|
||||||
Commit Queue is an experimental feature for the project which simplifies the
|
Commit Queue is an experimental feature for the project which simplifies the
|
||||||
landing process by automating it via GitHub Actions. With it, collaborators can
|
landing process by automating it via GitHub Actions. With it, collaborators can
|
||||||
land Pull Requests by adding the `commit-queue` label to a PR. All
|
land pull requests by adding the `commit-queue` label to a PR. All
|
||||||
checks will run via node-core-utils, and if the Pull Request is ready to land,
|
checks will run via node-core-utils, and if the pull request is ready to land,
|
||||||
the Action will rebase it and push to master.
|
the Action will rebase it and push to master.
|
||||||
|
|
||||||
This document gives an overview of how the Commit Queue works, as well as
|
This document gives an overview of how the Commit Queue works, as well as
|
||||||
@ -17,8 +17,8 @@ implementation details, reasoning for design choices, and current limitations.
|
|||||||
|
|
||||||
From a high-level, the Commit Queue works as follow:
|
From a high-level, the Commit Queue works as follow:
|
||||||
|
|
||||||
1. Collaborators will add `commit-queue` label to Pull Requests ready to land
|
1. Collaborators will add `commit-queue` label to pull requests ready to land
|
||||||
2. Every five minutes the queue will do the following for each Pull Request
|
2. Every five minutes the queue will do the following for each pull request
|
||||||
with the label:
|
with the label:
|
||||||
1. Check if the PR also has a `request-ci` label (if it has, skip this PR
|
1. Check if the PR also has a `request-ci` label (if it has, skip this PR
|
||||||
since it's pending a CI run)
|
since it's pending a CI run)
|
||||||
@ -40,10 +40,10 @@ From a high-level, the Commit Queue works as follow:
|
|||||||
## Current limitations
|
## Current limitations
|
||||||
|
|
||||||
The Commit Queue feature is still in early stages, and as such it might not
|
The Commit Queue feature is still in early stages, and as such it might not
|
||||||
work for more complex Pull Requests. These are the currently known limitations
|
work for more complex pull requests. These are the currently known limitations
|
||||||
of the commit queue:
|
of the commit queue:
|
||||||
|
|
||||||
1. All commits in a Pull Request must either be following commit message
|
1. All commits in a pull request must either be following commit message
|
||||||
guidelines or be a valid [`fixup!`](https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt)
|
guidelines or be a valid [`fixup!`](https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt)
|
||||||
commit that will be correctly handled by the [`--autosquash`](https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash)
|
commit that will be correctly handled by the [`--autosquash`](https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash)
|
||||||
option
|
option
|
||||||
@ -73,7 +73,7 @@ reasons:
|
|||||||
`node-core-utils` is configured with a personal token and
|
`node-core-utils` is configured with a personal token and
|
||||||
a Jenkins token from
|
a Jenkins token from
|
||||||
[@nodejs-github-bot](https://github.com/nodejs/github-bot).
|
[@nodejs-github-bot](https://github.com/nodejs/github-bot).
|
||||||
`octokit/graphql-action` is used to fetch all Pull Requests with the
|
`octokit/graphql-action` is used to fetch all pull requests with the
|
||||||
`commit-queue` label. The output is a JSON payload, so `jq` is used to turn
|
`commit-queue` label. The output is a JSON payload, so `jq` is used to turn
|
||||||
that into a list of PR ids we can pass as arguments to
|
that into a list of PR ids we can pass as arguments to
|
||||||
[`commit-queue.sh`](../../tools/actions/commit-queue.sh).
|
[`commit-queue.sh`](../../tools/actions/commit-queue.sh).
|
||||||
@ -87,8 +87,8 @@ that into a list of PR ids we can pass as arguments to
|
|||||||
1. The repository owner
|
1. The repository owner
|
||||||
2. The repository name
|
2. The repository name
|
||||||
3. The Action GITHUB_TOKEN
|
3. The Action GITHUB_TOKEN
|
||||||
4. Every positional argument starting at this one will be a Pull Request ID of
|
4. Every positional argument starting at this one will be a pull request ID of
|
||||||
a Pull Request with commit-queue set.
|
a pull request with commit-queue set.
|
||||||
|
|
||||||
The script will iterate over the pull requests. `ncu-ci` is used to check if
|
The script will iterate over the pull requests. `ncu-ci` is used to check if
|
||||||
the last CI is still pending, and calls to the GitHub API are used to check if
|
the last CI is still pending, and calls to the GitHub API are used to check if
|
||||||
|
@ -1,4 +1,4 @@
|
|||||||
# Pull Requests
|
# Pull requests
|
||||||
|
|
||||||
* [Dependencies](#dependencies)
|
* [Dependencies](#dependencies)
|
||||||
* [Setting up your local environment](#setting-up-your-local-environment)
|
* [Setting up your local environment](#setting-up-your-local-environment)
|
||||||
@ -108,7 +108,7 @@ $ git checkout -b my-branch -t upstream/master
|
|||||||
|
|
||||||
### Step 3: Code
|
### Step 3: Code
|
||||||
|
|
||||||
The vast majority of Pull Requests opened against the `nodejs/node`
|
The vast majority of pull requests opened against the `nodejs/node`
|
||||||
repository includes changes to one or more of the following:
|
repository includes changes to one or more of the following:
|
||||||
|
|
||||||
* the C/C++ code contained in the `src` directory
|
* the C/C++ code contained in the `src` directory
|
||||||
@ -145,7 +145,7 @@ C++ internals.
|
|||||||
|
|
||||||
It is a best practice to keep your changes as logically grouped
|
It is a best practice to keep your changes as logically grouped
|
||||||
as possible within individual commits. There is no limit to the number of
|
as possible within individual commits. There is no limit to the number of
|
||||||
commits any single Pull Request may have, and many contributors find it easier
|
commits any single pull request may have, and many contributors find it easier
|
||||||
to review changes that are split across multiple commits.
|
to review changes that are split across multiple commits.
|
||||||
|
|
||||||
```text
|
```text
|
||||||
@ -205,7 +205,7 @@ Refs: https://eslint.org/docs/rules/space-in-parens.html
|
|||||||
If you are new to contributing to Node.js, please try to do your best at
|
If you are new to contributing to Node.js, please try to do your best at
|
||||||
conforming to these guidelines, but do not worry if you get something wrong.
|
conforming to these guidelines, but do not worry if you get something wrong.
|
||||||
One of the existing contributors will help get things situated and the
|
One of the existing contributors will help get things situated and the
|
||||||
contributor landing the Pull Request will ensure that everything follows
|
contributor landing the pull request will ensure that everything follows
|
||||||
the project guidelines.
|
the project guidelines.
|
||||||
|
|
||||||
### Step 5: Rebase
|
### Step 5: Rebase
|
||||||
@ -234,7 +234,7 @@ often not clear where a new test file should go. When in doubt, add new tests
|
|||||||
to the `test/parallel/` directory and the right location will be sorted out
|
to the `test/parallel/` directory and the right location will be sorted out
|
||||||
later.
|
later.
|
||||||
|
|
||||||
Before submitting your changes in a Pull Request, always run the full Node.js
|
Before submitting your changes in a pull request, always run the full Node.js
|
||||||
test suite. To run the tests (including code linting) on Unix / macOS:
|
test suite. To run the tests (including code linting) on Unix / macOS:
|
||||||
|
|
||||||
```text
|
```text
|
||||||
@ -252,7 +252,7 @@ And on Windows:
|
|||||||
### Step 7: Push
|
### Step 7: Push
|
||||||
|
|
||||||
Once you are sure your commits are ready to go, with passing tests and linting,
|
Once you are sure your commits are ready to go, with passing tests and linting,
|
||||||
begin the process of opening a Pull Request by pushing your working branch to
|
begin the process of opening a pull request by pushing your working branch to
|
||||||
your fork on GitHub.
|
your fork on GitHub.
|
||||||
|
|
||||||
```text
|
```text
|
||||||
@ -261,23 +261,23 @@ $ git push origin my-branch
|
|||||||
|
|
||||||
### Step 8: Opening the pull request
|
### Step 8: Opening the pull request
|
||||||
|
|
||||||
From within GitHub, opening a new Pull Request will present you with a
|
From within GitHub, opening a new pull request will present you with a
|
||||||
[pull request template][]. Please try to do your best at filling out the
|
[pull request template][]. Please try to do your best at filling out the
|
||||||
details, but feel free to skip parts if you're not sure what to put.
|
details, but feel free to skip parts if you're not sure what to put.
|
||||||
|
|
||||||
Once opened, Pull Requests are usually reviewed within a few days.
|
Once opened, pull requests are usually reviewed within a few days.
|
||||||
|
|
||||||
### Step 9: Discuss and update
|
### Step 9: Discuss and update
|
||||||
|
|
||||||
You will probably get feedback or requests for changes to your Pull Request.
|
You will probably get feedback or requests for changes to your pull request.
|
||||||
This is a big part of the submission process so don't be discouraged! Some
|
This is a big part of the submission process so don't be discouraged! Some
|
||||||
contributors may sign off on the Pull Request right away, others may have
|
contributors may sign off on the pull request right away, others may have
|
||||||
more detailed comments or feedback. This is a necessary part of the process
|
more detailed comments or feedback. This is a necessary part of the process
|
||||||
in order to evaluate whether the changes are correct and necessary.
|
in order to evaluate whether the changes are correct and necessary.
|
||||||
|
|
||||||
To make changes to an existing Pull Request, make the changes to your local
|
To make changes to an existing pull request, make the changes to your local
|
||||||
branch, add a new commit with those changes, and push those to your fork.
|
branch, add a new commit with those changes, and push those to your fork.
|
||||||
GitHub will automatically update the Pull Request.
|
GitHub will automatically update the pull request.
|
||||||
|
|
||||||
```text
|
```text
|
||||||
$ git add my/changed/files
|
$ git add my/changed/files
|
||||||
@ -285,7 +285,7 @@ $ git commit
|
|||||||
$ git push origin my-branch
|
$ git push origin my-branch
|
||||||
```
|
```
|
||||||
|
|
||||||
It is also frequently necessary to synchronize your Pull Request with other
|
It is also frequently necessary to synchronize your pull request with other
|
||||||
changes that have landed in `master` by using `git rebase`:
|
changes that have landed in `master` by using `git rebase`:
|
||||||
|
|
||||||
```text
|
```text
|
||||||
@ -296,7 +296,7 @@ $ git push --force-with-lease origin my-branch
|
|||||||
|
|
||||||
**Important:** The `git push --force-with-lease` command is one of the few ways
|
**Important:** The `git push --force-with-lease` command is one of the few ways
|
||||||
to delete history in `git`. Before you use it, make sure you understand the
|
to delete history in `git`. Before you use it, make sure you understand the
|
||||||
risks. If in doubt, you can always ask for guidance in the Pull Request.
|
risks. If in doubt, you can always ask for guidance in the pull request.
|
||||||
|
|
||||||
If you happen to make a mistake in any of your commits, do not worry. You can
|
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).
|
amend the last commit (for example if you want to change the commit log).
|
||||||
@ -310,15 +310,15 @@ $ git push --force-with-lease origin my-branch
|
|||||||
There are a number of more advanced mechanisms for managing commits using
|
There are a number of more advanced mechanisms for managing commits using
|
||||||
`git rebase` that can be used, but are beyond the scope of this guide.
|
`git rebase` that can be used, but are beyond the scope of this guide.
|
||||||
|
|
||||||
Feel free to post a comment in the Pull Request to ping reviewers if you are
|
Feel free to post a comment in the pull request to ping reviewers if you are
|
||||||
awaiting an answer on something. If you encounter words or acronyms that
|
awaiting an answer on something. If you encounter words or acronyms that
|
||||||
seem unfamiliar, refer to this
|
seem unfamiliar, refer to this
|
||||||
[glossary](https://sites.google.com/a/chromium.org/dev/glossary).
|
[glossary](https://sites.google.com/a/chromium.org/dev/glossary).
|
||||||
|
|
||||||
#### Approval and request changes workflow
|
#### Approval and request changes workflow
|
||||||
|
|
||||||
All Pull Requests require "sign off" in order to land. Whenever a contributor
|
All pull requests require "sign off" in order to land. Whenever a contributor
|
||||||
reviews a Pull Request they may find specific details that they would like to
|
reviews a pull request they may find specific details that they would like to
|
||||||
see changed or fixed. These may be as simple as fixing a typo, or may involve
|
see changed or fixed. These may be as simple as fixing a typo, or may involve
|
||||||
substantive changes to the code you have written. While such requests are
|
substantive changes to the code you have written. While such requests are
|
||||||
intended to be helpful, they may come across as abrupt or unhelpful, especially
|
intended to be helpful, they may come across as abrupt or unhelpful, especially
|
||||||
@ -335,19 +335,19 @@ unhelpful is likely safe to ignore.
|
|||||||
|
|
||||||
### Step 10: Landing
|
### Step 10: Landing
|
||||||
|
|
||||||
In order to land, a Pull Request needs to be reviewed and [approved][] by
|
In order to land, a pull request needs to be reviewed and [approved][] by
|
||||||
at least two Node.js collaborators (one collaborator approval is enough if the
|
at least two Node.js Collaborators (one collaborator approval is enough if the
|
||||||
pull request has been open for more than 7 days) and pass a
|
pull request has been open for more than 7 days) and pass a
|
||||||
[CI (Continuous Integration) test run][]. After that, as long as there are no
|
[CI (Continuous Integration) test run][]. After that, as long as there are no
|
||||||
objections from other contributors, the Pull Request can be merged. If you find
|
objections from other contributors, the pull request can be merged. If you find
|
||||||
your Pull Request waiting longer than you expect, see the
|
your pull request waiting longer than you expect, see the
|
||||||
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).
|
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).
|
||||||
|
|
||||||
When a collaborator lands your Pull Request, they will post
|
When a collaborator lands your pull request, they will post
|
||||||
a comment to the Pull Request page mentioning the commit(s) it
|
a comment to the pull request page mentioning the commit(s) it
|
||||||
landed as. GitHub often shows the Pull Request as `Closed` at this
|
landed as. GitHub often shows the pull request as `Closed` at this
|
||||||
point, but don't worry. If you look at the branch you raised your
|
point, but don't worry. If you look at the branch you raised your
|
||||||
Pull Request against (probably `master`), you should see a commit with
|
pull request against (probably `master`), you should see a commit with
|
||||||
your name on it. Congratulations and thanks for your contribution!
|
your name on it. Congratulations and thanks for your contribution!
|
||||||
|
|
||||||
## Reviewing pull requests
|
## Reviewing pull requests
|
||||||
@ -356,17 +356,17 @@ All Node.js contributors who choose to review and provide feedback on Pull
|
|||||||
Requests have a responsibility to both the project and the individual making the
|
Requests have a responsibility to both the project and the individual making the
|
||||||
contribution. Reviews and feedback must be helpful, insightful, and geared
|
contribution. Reviews and feedback must be helpful, insightful, and geared
|
||||||
towards improving the contribution as opposed to simply blocking it. Do not
|
towards improving the contribution as opposed to simply blocking it. Do not
|
||||||
expect to be able to block a Pull Request from advancing simply because you say
|
expect to be able to block a pull request from advancing simply because you say
|
||||||
"No" without giving an explanation. Be open to having your mind changed. Be open
|
"No" without giving an explanation. Be open to having your mind changed. Be open
|
||||||
to working with the contributor to make the Pull Request better.
|
to working with the contributor to make the pull request better.
|
||||||
|
|
||||||
Reviews that are dismissive or disrespectful of the contributor or any other
|
Reviews that are dismissive or disrespectful of the contributor or any other
|
||||||
reviewers are strictly counter to the [Code of Conduct][].
|
reviewers are strictly counter to the [Code of Conduct][].
|
||||||
|
|
||||||
When reviewing a Pull Request, the primary goals are for the codebase to improve
|
When reviewing a pull request, the primary goals are for the codebase to improve
|
||||||
and for the person submitting the request to succeed. Even if a Pull Request
|
and for the person submitting the request to succeed. Even if a pull request
|
||||||
does not land, the submitters should come away from the experience feeling like
|
does not land, the submitters should come away from the experience feeling like
|
||||||
their effort was not wasted or unappreciated. Every Pull Request from a new
|
their effort was not wasted or unappreciated. Every pull request from a new
|
||||||
contributor is an opportunity to grow the community.
|
contributor is an opportunity to grow the community.
|
||||||
|
|
||||||
### Review a bit at a time
|
### Review a bit at a time
|
||||||
@ -391,8 +391,8 @@ Specific performance optimization techniques, coding styles and conventions
|
|||||||
change over time. The first impression you give to a new contributor never does.
|
change over time. The first impression you give to a new contributor never does.
|
||||||
|
|
||||||
Nits (requests for small changes that are not essential) are fine, but try to
|
Nits (requests for small changes that are not essential) are fine, but try to
|
||||||
avoid stalling the Pull Request. Most nits can typically be fixed by the
|
avoid stalling the pull request. Most nits can typically be fixed by the
|
||||||
Node.js collaborator landing the Pull Request but they can also be an
|
Node.js collaborator landing the pull request but they can also be an
|
||||||
opportunity for the contributor to learn a bit more about the project.
|
opportunity for the contributor to learn a bit more about the project.
|
||||||
|
|
||||||
It is always good to clearly indicate nits when you comment: e.g.
|
It is always good to clearly indicate nits when you comment: e.g.
|
||||||
@ -405,7 +405,7 @@ with the appropriate reason to keep the conversation flow concise and relevant.
|
|||||||
### Be aware of the person behind the code
|
### Be aware of the person behind the code
|
||||||
|
|
||||||
Be aware that *how* you communicate requests and reviews in your feedback can
|
Be aware that *how* you communicate requests and reviews in your feedback can
|
||||||
have a significant impact on the success of the Pull Request. Yes, we may land
|
have a significant impact on the success of the pull request. Yes, we may land
|
||||||
a particular change that makes Node.js better, but the individual might just
|
a particular change that makes Node.js better, but the individual might just
|
||||||
not want to have anything to do with Node.js ever again. The goal is not just
|
not want to have anything to do with Node.js ever again. The goal is not just
|
||||||
having good code.
|
having good code.
|
||||||
@ -416,7 +416,7 @@ There is a minimum waiting time which we try to respect for non-trivial
|
|||||||
changes, so that people who may have important input in such a distributed
|
changes, so that people who may have important input in such a distributed
|
||||||
project are able to respond.
|
project are able to respond.
|
||||||
|
|
||||||
For non-trivial changes, Pull Requests must be left open for at least 48 hours.
|
For non-trivial changes, pull requests must be left open for at least 48 hours.
|
||||||
Sometimes changes take far longer to review, or need more specialized review
|
Sometimes changes take far longer to review, or need more specialized review
|
||||||
from subject-matter experts. When in doubt, do not rush.
|
from subject-matter experts. When in doubt, do not rush.
|
||||||
|
|
||||||
@ -425,7 +425,7 @@ documentation, may be landed within the minimum 48 hour window.
|
|||||||
|
|
||||||
### Abandoned or stalled pull requests
|
### Abandoned or stalled pull requests
|
||||||
|
|
||||||
If a Pull Request appears to be abandoned or stalled, it is polite to first
|
If a pull request appears to be abandoned or stalled, it is polite to first
|
||||||
check with the contributor to see if they intend to continue the work before
|
check with the contributor to see if they intend to continue the work before
|
||||||
checking if they would mind if you took it over (especially if it just has
|
checking if they would mind if you took it over (especially if it just has
|
||||||
nits left). When doing so, it is courteous to give the original contributor
|
nits left). When doing so, it is courteous to give the original contributor
|
||||||
@ -437,10 +437,10 @@ commit.
|
|||||||
|
|
||||||
Any Node.js core collaborator (any GitHub user with commit rights in the
|
Any Node.js core collaborator (any GitHub user with commit rights in the
|
||||||
`nodejs/node` repository) is authorized to approve any other contributor's
|
`nodejs/node` repository) is authorized to approve any other contributor's
|
||||||
work. Collaborators are not permitted to approve their own Pull Requests.
|
work. Collaborators are not permitted to approve their own pull requests.
|
||||||
|
|
||||||
Collaborators indicate that they have reviewed and approve of the changes in
|
Collaborators indicate that they have reviewed and approve of the changes in
|
||||||
a Pull Request either by using GitHub's Approval Workflow, which is preferred,
|
a pull request either by using GitHub's Approval Workflow, which is preferred,
|
||||||
or by leaving an `LGTM` ("Looks Good To Me") comment.
|
or by leaving an `LGTM` ("Looks Good To Me") comment.
|
||||||
|
|
||||||
When explicitly using the "Changes requested" component of the GitHub Approval
|
When explicitly using the "Changes requested" component of the GitHub Approval
|
||||||
@ -458,8 +458,8 @@ Change requests that are vague, dismissive, or unconstructive may also be
|
|||||||
dismissed if requests for greater clarification go unanswered within a
|
dismissed if requests for greater clarification go unanswered within a
|
||||||
reasonable period of time.
|
reasonable period of time.
|
||||||
|
|
||||||
Use `Changes requested` to block a Pull Request from landing. When doing so,
|
Use `Changes requested` to block a pull request from landing. When doing so,
|
||||||
explain why you believe the Pull Request should not land along with an
|
explain why you believe the pull request should not land along with an
|
||||||
explanation of what may be an acceptable alternative course, if any.
|
explanation of what may be an acceptable alternative course, if any.
|
||||||
|
|
||||||
### Accept that there are different opinions about what belongs in Node.js
|
### Accept that there are different opinions about what belongs in Node.js
|
||||||
@ -484,7 +484,7 @@ ridiculed for even trying run counter to the [Code of Conduct][].
|
|||||||
|
|
||||||
Node.js has always optimized for speed of execution. If a particular change
|
Node.js has always optimized for speed of execution. If a particular change
|
||||||
can be shown to make some part of Node.js faster, it's quite likely to be
|
can be shown to make some part of Node.js faster, it's quite likely to be
|
||||||
accepted. Claims that a particular Pull Request will make things faster will
|
accepted. Claims that a particular pull request will make things faster will
|
||||||
almost always be met by requests for performance [benchmark results][] that
|
almost always be met by requests for performance [benchmark results][] that
|
||||||
demonstrate the improvement.
|
demonstrate the improvement.
|
||||||
|
|
||||||
@ -492,16 +492,16 @@ That said, performance is not the only factor to consider. Node.js also
|
|||||||
optimizes in favor of not breaking existing code in the ecosystem, and not
|
optimizes in favor of not breaking existing code in the ecosystem, and not
|
||||||
changing working functional code just for the sake of changing.
|
changing working functional code just for the sake of changing.
|
||||||
|
|
||||||
If a particular Pull Request introduces a performance or functional
|
If a particular pull request introduces a performance or functional
|
||||||
regression, rather than simply rejecting the Pull Request, take the time to
|
regression, rather than simply rejecting the pull request, take the time to
|
||||||
work *with* the contributor on improving the change. Offer feedback and
|
work *with* the contributor on improving the change. Offer feedback and
|
||||||
advice on what would make the Pull Request acceptable, and do not assume that
|
advice on what would make the pull request acceptable, and do not assume that
|
||||||
the contributor should already know how to do that. Be explicit in your
|
the contributor should already know how to do that. Be explicit in your
|
||||||
feedback.
|
feedback.
|
||||||
|
|
||||||
### Continuous integration testing
|
### Continuous integration testing
|
||||||
|
|
||||||
All Pull Requests that contain changes to code must be run through
|
All pull requests that contain changes to code must be run through
|
||||||
continuous integration (CI) testing at [https://ci.nodejs.org/][].
|
continuous integration (CI) testing at [https://ci.nodejs.org/][].
|
||||||
|
|
||||||
Only Node.js core collaborators with commit rights to the `nodejs/node`
|
Only Node.js core collaborators with commit rights to the `nodejs/node`
|
||||||
@ -514,18 +514,18 @@ This means that all tests pass and there are no linting errors. In reality,
|
|||||||
however, it is not uncommon for the CI infrastructure itself to fail on
|
however, it is not uncommon for the CI infrastructure itself to fail on
|
||||||
specific platforms or for so-called "flaky" tests to fail ("be red"). It is
|
specific platforms or for so-called "flaky" tests to fail ("be red"). It is
|
||||||
vital to visually inspect the results of all failed ("red") tests to determine
|
vital to visually inspect the results of all failed ("red") tests to determine
|
||||||
whether the failure was caused by the changes in the Pull Request.
|
whether the failure was caused by the changes in the pull request.
|
||||||
|
|
||||||
## Notes
|
## Notes
|
||||||
|
|
||||||
### Commit squashing
|
### Commit squashing
|
||||||
|
|
||||||
In most cases, do not squash commits that you add to your Pull Request during
|
In most cases, do not squash commits that you add to your pull request during
|
||||||
the review process. When the commits in your Pull Request land, they may be
|
the review process. When the commits in your pull request land, they may be
|
||||||
squashed into one commit per logical change. Metadata will be added to the
|
squashed into one commit per logical change. Metadata will be added to the
|
||||||
commit message (including links to the Pull Request, links to relevant issues,
|
commit message (including links to the pull request, links to relevant issues,
|
||||||
and the names of the reviewers). The commit history of your Pull Request,
|
and the names of the reviewers). The commit history of your pull request,
|
||||||
however, will stay intact on the Pull Request page.
|
however, will stay intact on the pull request page.
|
||||||
|
|
||||||
For the size of "one logical change",
|
For the size of "one logical change",
|
||||||
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
|
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
|
||||||
@ -535,9 +535,9 @@ when each individual commit lands on the master branch.
|
|||||||
|
|
||||||
### Getting approvals for your pull request
|
### Getting approvals for your pull request
|
||||||
|
|
||||||
A Pull Request is approved either by saying LGTM, which stands for
|
A pull request is approved either by saying LGTM, which stands for
|
||||||
"Looks Good To Me", or by using GitHub's Approve button.
|
"Looks Good To Me", or by using GitHub's Approve button.
|
||||||
GitHub's Pull Request review feature can be used during the process.
|
GitHub's pull request review feature can be used during the process.
|
||||||
For more information, check out
|
For more information, check out
|
||||||
[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g)
|
[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g)
|
||||||
or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/).
|
or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/).
|
||||||
@ -548,20 +548,20 @@ because the reviewers have hit the buttons before.
|
|||||||
|
|
||||||
### CI testing
|
### CI testing
|
||||||
|
|
||||||
Every Pull Request needs to be tested
|
Every pull request needs to be tested
|
||||||
to make sure that it works on the platforms that Node.js
|
to make sure that it works on the platforms that Node.js
|
||||||
supports. This is done by running the code through the CI system.
|
supports. This is done by running the code through the CI system.
|
||||||
|
|
||||||
Only a collaborator can start a CI run. Usually one of them will do it
|
Only a collaborator can start a CI run. Usually one of them will do it
|
||||||
for you as approvals for the Pull Request come in.
|
for you as approvals for the pull request come in.
|
||||||
If not, you can ask a collaborator to start a CI run.
|
If not, you can ask a collaborator to start a CI run.
|
||||||
|
|
||||||
### Waiting until the pull request gets landed
|
### Waiting until the pull request gets landed
|
||||||
|
|
||||||
A Pull Request needs to stay open for at least 48 hours from when it is
|
A pull request needs to stay open for at least 48 hours from when it is
|
||||||
submitted, even after it gets approved and passes the CI. This is to make sure
|
submitted, even after it gets approved and passes the CI. This is to make sure
|
||||||
that everyone has a chance to weigh in. If the changes are trivial,
|
that everyone has a chance to weigh in. If the changes are trivial,
|
||||||
collaborators may decide it doesn't need to wait. A Pull Request may well take
|
collaborators may decide it doesn't need to wait. A pull request may well take
|
||||||
longer to be merged in. All these precautions are important because Node.js is
|
longer to be merged in. All these precautions are important because Node.js is
|
||||||
widely used, so don't be discouraged!
|
widely used, so don't be discouraged!
|
||||||
|
|
||||||
|
@ -422,7 +422,7 @@ chunkLen encoding rate confidence.interval
|
|||||||
|
|
||||||
### Running benchmarks on the CI
|
### Running benchmarks on the CI
|
||||||
|
|
||||||
To see the performance impact of a Pull Request by running benchmarks on
|
To see the performance impact of a pull request by running benchmarks on
|
||||||
the CI, check out [How to: Running core benchmarks on Node.js CI][benchmark-ci].
|
the CI, check out [How to: Running core benchmarks on Node.js CI][benchmark-ci].
|
||||||
|
|
||||||
## Creating a benchmark
|
## Creating a benchmark
|
||||||
|
@ -184,7 +184,7 @@ The project has two venues for real-time discussion:
|
|||||||
|
|
||||||
## Landing PRs
|
## Landing PRs
|
||||||
|
|
||||||
See the Collaborator Guide: [Landing Pull Requests][].
|
See the Collaborator Guide: [Landing pull requests][].
|
||||||
|
|
||||||
Commits in one PR that belong to one logical change should
|
Commits in one PR that belong to one logical change should
|
||||||
be squashed. It is rarely the case in onboarding exercises, so this
|
be squashed. It is rarely the case in onboarding exercises, so this
|
||||||
@ -240,7 +240,7 @@ needs to be pointed out separately during the onboarding.
|
|||||||
|
|
||||||
[Code of Conduct]: https://github.com/nodejs/admin/blob/HEAD/CODE_OF_CONDUCT.md
|
[Code of Conduct]: https://github.com/nodejs/admin/blob/HEAD/CODE_OF_CONDUCT.md
|
||||||
[Labels]: doc/guides/collaborator-guide.md#labels
|
[Labels]: doc/guides/collaborator-guide.md#labels
|
||||||
[Landing Pull Requests]: doc/guides/collaborator-guide.md#landing-pull-requests
|
[Landing pull requests]: doc/guides/collaborator-guide.md#landing-pull-requests
|
||||||
[Publicizing or hiding organization membership]: https://help.github.com/articles/publicizing-or-hiding-organization-membership/
|
[Publicizing or hiding organization membership]: https://help.github.com/articles/publicizing-or-hiding-organization-membership/
|
||||||
[`author-ready`]: doc/guides/collaborator-guide.md#author-ready-pull-requests
|
[`author-ready`]: doc/guides/collaborator-guide.md#author-ready-pull-requests
|
||||||
[`core-validate-commit`]: https://github.com/nodejs/core-validate-commit
|
[`core-validate-commit`]: https://github.com/nodejs/core-validate-commit
|
||||||
|
Loading…
x
Reference in New Issue
Block a user