Skip to content

Commit e9d9a4c

Browse files
committed
doc: address review feedback on large pull requests guide
- Exclude routine dependency/WPT/bot PRs from the policy - Replace design document requirement with detailed PR description - Clarify dependency commit ordering for squash landing - Remove splitting strategies that contradict self-contained PRs - Add links from CONTRIBUTING.md, pull-requests.md, collaborator-guide.md Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
1 parent 28a1d11 commit e9d9a4c

File tree

4 files changed

+40
-13
lines changed

4 files changed

+40
-13
lines changed

CONTRIBUTING.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ dependencies, and tools contained in the `nodejs/node` repository.
4545
* [Setting up your local environment](./doc/contributing/pull-requests.md#setting-up-your-local-environment)
4646
* [The Process of Making Changes](./doc/contributing/pull-requests.md#the-process-of-making-changes)
4747
* [Reviewing Pull Requests](./doc/contributing/pull-requests.md#reviewing-pull-requests)
48+
* [Large Pull Requests](./doc/contributing/large-pull-requests.md)
4849
* [Notes](./doc/contributing/pull-requests.md#notes)
4950

5051
## Developer's Certificate of Origin 1.1

doc/contributing/collaborator-guide.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,9 @@ Pay special attention to pull requests for dependencies which have not
132132
been automatically generated and follow the guidance in
133133
[Maintaining Dependencies](https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies).
134134

135+
Pull requests that exceed 3000 lines of changes have additional requirements.
136+
See the [large pull requests][] guide.
137+
135138
In some cases, it might be necessary to summon a GitHub team to a pull request
136139
for review by @-mention.
137140
See [Who to CC in the issue tracker](#who-to-cc-in-the-issue-tracker).
@@ -1063,6 +1066,7 @@ need to be attached anymore, as only important bugfixes will be included.
10631066
[commit message guidelines]: pull-requests.md#commit-message-guidelines
10641067
[commit-example]: https://github.com/nodejs/node/commit/b636ba8186
10651068
[commit-queue.md]: ./commit-queue.md
1069+
[large pull requests]: large-pull-requests.md
10661070
[freebsd]: https://github.com/orgs/nodejs/teams/platform-freebsd
10671071
[git-email]: https://help.github.com/articles/setting-your-commit-email-address-in-git/
10681072
[git-node]: https://github.com/nodejs/node-core-utils/blob/HEAD/docs/git-node.md

doc/contributing/large-pull-requests.md

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* [What qualifies as a large pull request](#what-qualifies-as-a-large-pull-request)
55
* [Who can open a large pull request](#who-can-open-a-large-pull-request)
66
* [Requirements](#requirements)
7-
* [Design document](#design-document)
7+
* [Detailed pull request description](#detailed-pull-request-description)
88
* [Review guide](#review-guide)
99
* [Approval requirements](#approval-requirements)
1010
* [Dependency changes](#dependency-changes)
@@ -33,6 +33,19 @@ request, including changes in `deps/`, `test/`, `doc/`, `lib/`, `src/`, and
3333
Changes in `deps/` are included in this count. Dependency changes are
3434
sensitive because they often receive less scrutiny than first-party code.
3535

36+
The following categories of pull requests are **excluded** from this policy,
37+
even if they exceed the line threshold:
38+
39+
* Routine dependency updates (e.g., V8, ICU, undici, uvwasi) generated by
40+
automation or performed by collaborators following the standard dependency
41+
update process.
42+
* Web Platform Tests (WPT) imports and updates.
43+
* Other bot-issued or automated pull requests (e.g., license updates, test
44+
fixture regeneration).
45+
46+
These pull requests already have established review processes and do not
47+
benefit from the additional requirements described here.
48+
3649
## Who can open a large pull request
3750

3851
Large pull requests may only be opened by existing
@@ -47,20 +60,19 @@ find a collaborator to champion the work.
4760
All large pull requests must satisfy the following requirements in addition to
4861
the standard [pull request requirements](./pull-requests.md).
4962

50-
### Design document
63+
### Detailed pull request description
5164

52-
A design document or detailed issue must be available when the pull request is
53-
opened. The design document should explain:
65+
The pull request description must provide sufficient context for reviewers
66+
to understand the change. The description should explain:
5467

5568
* The motivation for the change.
5669
* The high-level approach and architecture.
5770
* Any alternatives that were considered and why they were rejected.
5871
* How the change interacts with existing subsystems.
5972

60-
The design document can be a GitHub issue, a Markdown file in the repository,
61-
or an external document linked from the pull request description. It does not
62-
need to be shared before the work starts, but it must be available for reviewers
63-
when the pull request is opened.
73+
A thorough pull request description is sufficient. There is no requirement
74+
to produce a separate design document, although contributors may choose to
75+
link to a GitHub issue or other discussion where the design was developed.
6476

6577
### Review guide
6678

@@ -86,9 +98,12 @@ Large pull requests follow the same approval path as semver-major changes:
8698

8799
When a large pull request adds or modifies a dependency in `deps/`:
88100

89-
* Dependency changes must be in a **separate commit** from the rest of the
101+
* Dependency changes should be in a **separate commit** from the rest of the
90102
pull request. This makes it easier to review the dependency update
91-
independently from the first-party code changes.
103+
independently from the first-party code changes. When the pull request is
104+
squashed on landing, the dependency commit should be the one that carries
105+
the squashed commit message, so that `git log` clearly reflects the
106+
overall change.
92107
* The provenance and integrity of the dependency must be verifiable.
93108
Include documentation of how the dependency was obtained and how
94109
reviewers can reproduce the build artifact.
@@ -99,10 +114,12 @@ Contributors should always consider whether a large pull request can be split
99114
into smaller, independently reviewable pieces. Strategies include:
100115

101116
* Landing foundational internal APIs first, then building on top of them.
102-
* Separating test additions from implementation changes.
103-
* Splitting documentation into its own pull request.
104117
* Landing refactoring or preparatory changes before the main feature.
105118

119+
Each pull request in a split series should remain self-contained: it should
120+
include the implementation, tests, and documentation needed for that piece
121+
to stand on its own.
122+
106123
### Feature forks and branches
107124

108125
For extremely large or complex changes that develop over time, such as adding
@@ -131,7 +148,8 @@ request.
131148
Reviewing a large pull request is a significant time investment. Reviewers
132149
should:
133150

134-
* Read the design document and review guide before diving into the code.
151+
* Read the pull request description and review guide before diving into the
152+
code.
135153
* Focus review effort on `lib/` and `src/` changes, which have the highest
136154
impact on the runtime. `test/` and `doc/` changes, while important, are
137155
lower risk.

doc/contributing/pull-requests.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,9 @@ From within GitHub, opening a new pull request will present you with a
289289
[pull request template][]. Please try to do your best at filling out the
290290
details, but feel free to skip parts if you're not sure what to put.
291291

292+
If your pull request exceeds 3000 lines of changes, see the
293+
[large pull requests][] guide for additional requirements.
294+
292295
Once opened, pull requests are usually reviewed within a few days.
293296

294297
To get feedback on your proposed change even though it is not ready
@@ -614,4 +617,5 @@ More than one subsystem may be valid for any particular issue or pull request.
614617
[maintaining dependencies]: ./maintaining/maintaining-dependencies.md
615618
[nodejs/core-validate-commit]: https://github.com/nodejs/core-validate-commit/blob/main/lib/rules/subsystem.js
616619
[pull request template]: https://raw.githubusercontent.com/nodejs/node/HEAD/.github/PULL_REQUEST_TEMPLATE.md
620+
[large pull requests]: large-pull-requests.md
617621
[running tests]: ../../BUILDING.md#running-tests

0 commit comments

Comments
 (0)