Skip to content
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

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
**Connections**
_Link to the issues addressed by this PR, or dependent PRs in other repositories_

<i>When one pull request builds on another, please put "Depends on
#NNNN" 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 PR status to indicate this.</i>

**Description**
_Describe what problem this is solving, and how it's solved._

**Testing**
_Explain how this change is tested._

**Squash or Rebase?**

<i>If your 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
latter case, please ensure that each commit passes all CI tests, so
that we can continue to bisect along `trunk` to isolate bugs.</i>

<!--
Thanks for filing! The codeowners file will automatically request reviews from the appropriate teams.

Expand Down
77 changes: 70 additions & 7 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.