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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+46 −8
Open

CONTRIBUTING.md: Fill out section on pull requests. #6879

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 46 additions & 8 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,49 @@ 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…

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.
### Pull requests

The "Assigned" field on a pull request indicates who has taken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Let's use backticks (i.e., `Assigned`) instead of double quotes to make it clear that this is content that somebody would write.

This feedback applies to other references to fields or suggested content fragments.

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 the "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
latter case, please ensure that each commit passes all CI tests, so
that we can continue to bisect along `trunk` to isolate bugs.
Comment on lines +135 to +144
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put both of these in the PR template? Feel free to make a separate PR if needed.


Pull requests should not contain merge commits. Please rebase on
`trunk` before submitting a pull request. (Draft pull requests may
contain anything you like.)
Comment on lines +146 to +148
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have this rule across the board because of the added friction - I think squash-merging is totally fine for any PR that needs to be evaluated as a whole.


#### Large pull requests are not accepted

Large projects must be broken into series of smaller pull requests
Comment on lines +150 to +152
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe like this?

Suggested change
#### Large pull requests are not accepted
Large projects must be broken into series of smaller pull requests
#### Large pull requests are not normally accepted
If you believe a project you are working on requires a large pull request then you *must* ask a maintainer before. If they believe that it requires a large pull request then you should keep in frequent contact and talk to them if the plan changes. Otherwise large projects must be broken into series of smaller pull requests

otherwise it could confuse beginners (which I am assuming this is for) if they see a large project.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: This conversation branches out from the previous one: #6879 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't set that comment up properly to suggest.

that can be reviewed and discussed independently. WGPU's experience
with large, complex pull requests has been bad enough that we now
simply reject them, without trying to assess their merits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to things that effectively need large PRs? To use an example: ray-tracing where (most) of it was a single feature that couldn't be broken down? Some of these statements are definite, so maybe they should note where it's a requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I agree with @Vecvec that an unqualified "if big, straight to jail /s" rule is not easy for an external contributor to reason about. I also think that we need to disentangle (1) making aligned changed from (2) the art of making review easier, which are related but distinct. I think our intent would be better communicated with something like this instead:

WGPU is a complex codebase, and contributing to it is often nontrivial. To best ensure that your contributions are accepted, we have some recommendations to consider before you get started:

  • Large efforts that are ultimately rejected tend to burn contributors out on both sides of a review. To avoid this, we strongly encourage you to validate time-consuming contributions by engaging maintainership before you invest yourself too heavily.
  • File the smallest PRs that make sense on their own. Using atomic commits is also encouraged, since they facilitate review on both sides.

Copy link
Member

@ErichDonGubler ErichDonGubler Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This information is technically duplicative with information in the "What can I work on"? as a new contributor? section.

suggestion: Keeping this dedicated section and linking to it from others would reconcile this.


- Large pull requests are very 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 giant 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. Giant pull requests effectively arrogate the ability to
make design decisions from the maintainers, whereas incremental
changes can be discussed and revised without drama.
Loading