doc: add large pull requests contributing guide#62829
doc: add large pull requests contributing guide#62829mcollina wants to merge 13 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
There was a problem hiding this comment.
- We should link to it from CONTRIBUTING.md, pull-requests.md, collaborator-guide.md (or some subset of those)
- We should exclude routine dependency/wpt/test bot-issued PRs, this can either be excluded explicitly with additional rules or by changing the classification put forth by the
OvervieworWhat qualifies as a large pull requestsections - dependency changes in a separate commit is at odds with having each commit self-contained, this is fine when the commits get squashed but the first commit's message is used for the squash so the first commit would be the non-deps changes? seems odd and counter-intuitive
- Design document - I 100% discourage linking outside documents, a detailed PR description should also suffice, 3000 can easily be hit with just thorough test coverage for an otherwise trivial change, how would a design document help such PRs?
- Both
Separating test additions from implementation changes.andSplitting documentation into its own pull request.strategies to avoiding large PRs are contrary to what we usually request of contributions. Those strategies only work when the contribution isn't "done" or "shipped" and is excluded from builds.
e9d9a4c to
7552c50
Compare
|
The specification here doesn't seem very clear:
|
We have to draw a line, and this is as good as any. I proposed 5k but I was asked to lower it. I would be happier with a 3k of non-test/non-doc changes, but that would require better tooling, which I'd prefer to avoid creating now. What would work for you?
The number that matters is at landing time. |
In the document it says: So
I would say as soon as it reaches the threshold the new rules apply. This also means that if with further commits the PR shrinks, rules do not apply anymore. |
|
FYI, I've added the new |
|
|
|
@nodejs/tsc ... we could use some more TSC scrutiny on this policy change. |
57506d2 to
104bc61
Compare
To be honest I don't have strong opinions on this. I just wrote out how I interpreted the rule to make sure we're on the same page. Thinking about it, my formula above probably would easily trigger this limit. Just making out an example, if I have a JS function which body is 100 lines long and I wrap the entire body inside a So either we only count additions or we raised the combined limit. |
|
I've updated it to to be ADDED - DELETED > LIMIT |
407ac3f to
2d0bc1d
Compare
|
Linter is failing. Run |
Commit Queue failed- Loading data for nodejs/node/pull/62829 ✔ Done loading data for nodejs/node/pull/62829 ----------------------------------- PR info ------------------------------------ Title doc: add large pull requests contributing guide (#62829) Author Matteo Collina <matteo.collina@gmail.com> (@mcollina) Branch mcollina:doc/large-pull-requests -> nodejs:main Labels doc, commit-queue-squash Commits 13 - doc: add large pull requests contributing guide - doc: address review feedback on large pull requests guide - doc: add large pull requests contributing guide - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Update doc/contributing/large-pull-requests.md - Apply suggestions from code review - Merge remote-tracking branch 'origin/doc/large-pull-requests' into do… Committers 2 - Matteo Collina <hello@matteocollina.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/62829 Fixes: https://github.com/nodejs/node/issues/62752 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Ruy Adorno <ruy@vlt.sh> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/62829 Fixes: https://github.com/nodejs/node/issues/62752 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Ruy Adorno <ruy@vlt.sh> -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 19 Apr 2026 10:35:59 GMT ✔ Approvals: 9 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4252771875 ✔ - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/62829#pullrequestreview-4255972855 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4142929050 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4158756701 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4173059266 ✔ - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4241055149 ✔ - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4196309267 ✔ - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/62829#pullrequestreview-4252659143 ✔ - Ruy Adorno (@ruyadorno) (TSC): https://github.com/nodejs/node/pull/62829#pullrequestreview-4255541403 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 62829 From https://github.com/nodejs/node * branch refs/pull/62829/merge -> FETCH_HEAD ✔ Fetched commits as e15f90559bc6..dc1a57087b4e -------------------------------------------------------------------------------- [main 101365c922] doc: add large pull requests contributing guide Author: Matteo Collina <hello@matteocollina.com> Date: Sun Apr 19 12:35:40 2026 +0200 1 file changed, 140 insertions(+) create mode 100644 doc/contributing/large-pull-requests.md Auto-merging CONTRIBUTING.md Auto-merging doc/contributing/pull-requests.md [main 5d388bd498] doc: address review feedback on large pull requests guide Author: Matteo Collina <hello@matteocollina.com> Date: Sun Apr 19 15:35:13 2026 +0200 4 files changed, 40 insertions(+), 13 deletions(-) Auto-merging CONTRIBUTING.md Auto-merging doc/contributing/collaborator-guide.md CONFLICT (content): Merge conflict in doc/contributing/collaborator-guide.md Auto-merging doc/contributing/large-pull-requests.md CONFLICT (add/add): Merge conflict in doc/contributing/large-pull-requests.md Auto-merging doc/contributing/pull-requests.md CONFLICT (content): Merge conflict in doc/contributing/pull-requests.md error: could not apply 2d0bc1d9df... doc: add large pull requests contributing guide hint: After resolving the conflicts, mark them with hint: "git add/rm <pathspec>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort". ✘ Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/25625476444 |
- 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>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com> Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
dc1a570 to
a15a5dc
Compare
|
Unfortunately, looks like there are linting issues preventing landing. |
Fixes: #62752
Adds a contributing guide for large pull requests (5000+ lines), covering: