CONTRIBUTING.md: Fill out section on pull requests. #6879
Conversation
| 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. |
There was a problem hiding this comment.
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.) |
There was a problem hiding this comment.
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 projects must be broken into series of smaller pull requests | ||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 not accepted | ||
|
|
||
| Large projects must be broken into series of smaller pull requests |
There was a problem hiding this comment.
Maybe like this?
| #### 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.
There was a problem hiding this comment.
Context: This conversation branches out from the previous one: #6879 (comment)
There was a problem hiding this comment.
I didn't set that comment up properly to suggest.
| PR. | ||
| ### Pull requests | ||
|
|
||
| The "Assigned" field on a pull request indicates who has taken |
There was a problem hiding this comment.
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.
|
I think I'm going to be able to continue reviewing this on Tuesday of this coming week. 🫡 See you then! |
No description provided.