Skip to content

Commit 7cbad8e

Browse files
authored
CONTRIBUTING.md: Fill out section on pull requests. (#6879)
1 parent f98bd60 commit 7cbad8e

File tree

2 files changed

+92
-8
lines changed

2 files changed

+92
-8
lines changed

.github/pull_request_template.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,25 @@
11
**Connections**
22
_Link to the issues addressed by this PR, or dependent PRs in other repositories_
33

4+
_When one pull request builds on another, please put "Depends on
5+
#NNNN" towards the top of its description. This helps maintainers
6+
notice that they shouldn't merge it until its ancestor has been
7+
approved. Don't use draft PR status to indicate this._
8+
49
**Description**
510
_Describe what problem this is solving, and how it's solved._
611

712
**Testing**
813
_Explain how this change is tested._
914

15+
**Squash or Rebase?**
16+
17+
_If your pull request contains multiple commits, please indicate whether
18+
they need to be squashed into a single commit before they're merged,
19+
or if they're ready to rebase onto `trunk` as they stand. In the
20+
latter case, please ensure that each commit passes all CI tests, so
21+
that we can continue to bisect along `trunk` to isolate bugs._
22+
1023
<!--
1124
Thanks for filing! The codeowners file will automatically request reviews from the appropriate teams.
1225

CONTRIBUTING.md

Lines changed: 79 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,85 @@ TODO
135135
into fixing and (2) otherwise isn't being prioritized are likely to
136136
be closed.
137137

138-
### What to expect when you submit a PR
139-
140-
TODO: It is strongly recommended that you validate your contributions before
141-
you make significant efforts…
142-
143-
The "Assigned" field on a PR indicates who has taken responsibility
144-
for reviewing the PR, not who is responsible for the content of the
145-
PR.
138+
### Pull requests
146139

147140
You can see some common things that PR reviewers are going to look for in
148141
[`docs/review-checklist.md`].
142+
143+
A draft pull request is taken to be not yet ready for review. Marking
144+
drafts as such helps the maintainers triage review work.
145+
146+
The `Assigned` field on a pull request indicates who has taken
147+
responsibility for shepherding it through the review process, not who
148+
is responsible for authoring it. The assignee is usually the reviewer,
149+
but they can also delegate the review to someone else. The intent of
150+
assignment is simply to ensure that pull requests don't get neglected.
151+
152+
#### Designing new features
153+
154+
As an open source project, WGPU wants to serve a broad audience. This
155+
helps us cast a wide net for contributors, and widens the impact of
156+
their work. However, WGPU does not promise to incorporate every
157+
proposed feature.
158+
159+
Large efforts that are ultimately rejected tend to burn contributors
160+
out on both sides of a review. To avoid this, we strongly encourage
161+
you to validate time-consuming contributions by engaging
162+
maintainership before you invest yourself too heavily. Try to build a
163+
consensus on the approach, including API changes, shader language
164+
extensions, implementation architecture, error handling, testing
165+
plans, benchmarking, and so on.
166+
167+
#### Large pull requests are risky
168+
169+
Contributors should anticipate that the larger and more complex a pull
170+
request is, the less likely it is that reviewers will accept it,
171+
regardless of its merits.
172+
173+
The WGPU project has had poor experiences with large, complex pull
174+
requests:
175+
176+
- Complex pull requests are difficult to review effectively. It is
177+
common for us to debug a problem in WGPU and find that it was
178+
introduced by some massive pull request that we had reviewed and
179+
accepted, showing that we obviously hadn't understood it as well as
180+
we'd thought.
181+
182+
- A large, complex pull request obviously represents a significant
183+
effort on the part of the author. At a personal level, it is quite
184+
stressful to question its design decisions, knowing that changing
185+
them will require the author to essentially reimplement the project
186+
from scratch. Such pull requests make it hard for maintainers to
187+
uphold their responsibility to keep WGPU maintainable. Incremental
188+
changes are easier to discuss and revise without drama.
189+
190+
These problems are serious enough that maintainers may choose to
191+
reject large, complex pull requests, regardless of the value of the
192+
feature or the technical merit of the code.
193+
194+
The problem isn't really the *size* of the pull request: a simple
195+
rename, with no changes to functionality, might touch hundreds of
196+
files, but be easy to review. Or, a change to Naga might affect dozens
197+
of snapshot test output files, without being hard to understand.
198+
199+
Rather, the problem is the *complexity* of the pull request: how many
200+
moving pieces does the reviewer need to assess at once? In our
201+
experience, almost every large change can be pared down by separating
202+
out:
203+
204+
- Preparatory refactors that are at least harmless in isolation, and
205+
perhaps beneficial.
206+
207+
- Helpers and utilities that can be used elsewhere in the code base,
208+
even if they don't show their full value until the whole thing is
209+
merged.
210+
211+
- Renames and code motion with no semantic effect, like changes to
212+
types or behavior. When putting these in a separate pull request
213+
would be awkward, they should at least be segregated into their own
214+
commits within a pull request.
215+
216+
Brevity for brevity's sake is not the goal. Rather, the goal is to
217+
help the reviewer anticipate the changes' consequences. When a pull
218+
request addresses only a single issue, even if it is textually large,
219+
a trustworthy review becomes more achievable.

0 commit comments

Comments
 (0)