-
Notifications
You must be signed in to change notification settings - Fork 987
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
CONTRIBUTING.md: Fill out section on pull requests. #6879
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,11 +121,74 @@ TODO | |
into fixing and (2) otherwise isn't being prioritized are likely to | ||
be closed. | ||
|
||
### What to expect when you submit a PR | ||
|
||
TODO: It is strongly recommended that you validate your contributions before | ||
you make significant efforts… | ||
### Pull requests | ||
|
||
The `Assigned` field on a pull request indicates who has taken | ||
responsibility for shepherding it through the review process, not who | ||
is responsible for authoring it. The assignee is usually the reviewer, | ||
but they can also delegate the review to someone else. The intent of | ||
assignment is simply to ensure that pull requests don't get neglected. | ||
|
||
A draft pull request is taken to be not yet ready for review. Marking | ||
drafts as such helps the maintainers triage review work. | ||
|
||
When one pull request builds on another, please put `Depends on #NNNN` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick(non-blocking): The request to write I'd accept handling this as follow-up, since this might require some non-trivial editing. |
||
towards the top of its description. This helps maintainers notice that | ||
they shouldn't merge it until its ancestor has been approved. Don't | ||
use draft pull request status to indicate dependency. | ||
|
||
If a pull request contains multiple commits, please indicate whether | ||
they need to be squashed into a single commit before they're merged, | ||
or if they're ready to rebase onto `trunk` as they stand. In the | ||
Comment on lines
+140
to
+142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I think this should be in the PR template to stop people (like me) forgetting about it. Maybe even in the template text (not just a comment) to act as a default (I would assume it would be squash). I also think a default would be especially useful for beginners too. |
||
latter case, please ensure that each commit passes all CI tests, so | ||
that we can continue to bisect along `trunk` to isolate bugs. | ||
|
||
#### Large pull requests are very risky | ||
|
||
If you are working on a large project, you *must* communicate with the | ||
wgpu maintainers in advance to build a consensus on your approach, | ||
including API changes, shader language extensions, implementation | ||
architecture, error handling, testing plans, benchmarking, and so on. | ||
|
||
The WGPU project has repeatedly had bad experiences with large, | ||
complex pull requests: | ||
|
||
- Complex pull requests are difficult to review effectively. It is | ||
common for us to debug a problem in WGPU and find that it was | ||
introduced by some massive pull request that we had accepted, | ||
showing that we obviously hadn't understood it as well as we'd | ||
thought. | ||
|
||
- A large, complex pull request obviously represents a significant | ||
effort on the part of the author. At a personal level, it is quite | ||
stressful to question its design decisions, knowing that changing | ||
them will require the author to essentially reimplement the project | ||
from scratch. Such pull requests effectively arrogate the ability to | ||
make design decisions from the maintainers, whereas incremental | ||
changes can be discussed and revised without drama. | ||
|
||
These problems are serious enough that the WGPU project may reject | ||
such PRs, at the maintainer's discretion, regardless of the value of | ||
the feature or the technical merit of the code. | ||
|
||
The problem isn't really the *size* of the pull request: a simple | ||
rename, with no changes to functionality, might touch hundreds of | ||
files, but be easy to review. Or, a change to Naga might affect dozens | ||
of snapshot test output files, without being hard to understand. | ||
|
||
Rather, the problem is the *complexity* of the pull request: how many | ||
moving pieces does the reviewer need to assess at once? In our | ||
experience, almost every large change can be pared down by separating | ||
out: | ||
|
||
- Preparatory refactors that are at least harmless in isolation, and | ||
perhaps beneficial. | ||
|
||
- Helpers and utilities that can be used elsewhere in the code base, | ||
even if they don't show their true value until the whole thing is | ||
merged. | ||
|
||
- Code motion and renames, without changing functionality. | ||
|
||
The "Assigned" field on a PR indicates who has taken responsibility | ||
for reviewing the PR, not who is responsible for the content of the | ||
PR. | ||
When the remaining change addresses only a single issue, even if it is | ||
large, a trustworthy review becomes more achievable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: This would be more idiomatic Markdown to use
_content_
, rather than<i>content</i>
. I don't see anything here that would interfere with that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also applies to the next added paragraph.