1# Code Review 2 3We only merge changes submitted as GitHub Pull Requests, and only after they've 4been approved by at least one Core Team reviewer who did not author the PR. This 5section covers expectations for the people performing those reviews. These 6guidelines are in addition to expectations which apply to everyone in the 7community, such as following the Code of Conduct. 8 9It is our goal to respond to every contribution in a timely fashion. Although we 10make no guarantees, we aim to usually provide some kind of response within about 11one business day. 12 13That's important because we appreciate all the contributions we receive, made by 14a diverse collection of people from all over the world. One way to show our 15appreciation, and our respect for the effort that goes into contributing to this 16project, is by not keeping contributors waiting. It's no fun to submit a pull 17request and then sit around wondering if anyone is ever going to look at it. 18 19That does not mean we will review every PR promptly, let alone merge them. Some 20contributions have needed weeks of discussion and changes before they were ready 21to merge. For some other contributions, we've had to conclude that we could not 22merge them, no matter how much we appreciate the effort that went into them. 23 24What this does mean is that we will communicate with each contributor to set 25expectations around the review process. Some examples of good communication are: 26 27- "I intend to review this but I can't yet. Please leave me a message if I 28 haven't responded by (a specific date in the near future)." 29 30- "I think (a specific other contributor) should review this." 31 32- "I'm having difficulty reviewing this PR because of (a specific reason, if 33 it's something the contributor might reasonably be able to help with). Are you 34 able to change that? If not, I'll ask my colleagues for help (or some other 35 concrete resolution)." 36 37If you are able to quickly review the PR, of course, you can just do that. 38 39You can find open Wasmtime pull requests for which your review has been 40requested with this search: 41 42<https://github.com/bytecodealliance/wasmtime/pulls?q=is:open+type:pr+user-review-requested:@me> 43 44## Auto-assigned reviewers 45 46We automatically assign a reviewer to every newly opened pull request. We do 47this to avoid the problem of diffusion of responsibility, where everyone thinks 48somebody else will respond to the PR, so nobody does. 49 50To be in the pool of auto-assigned reviewers, a Core Team member must commit to 51following the above goals and guidelines around communicating in a timely 52fashion. 53 54We don't ask everyone to make this commitment. In particular, we don't believe 55it's fair to expect quick responses from unpaid contributors, although we 56gratefully accept any review work they do have time to contribute. 57 58If you are in the auto-assignment pool, remember: **You are not necessarily 59expected to review the pull requests which are assigned to you.** Your only 60responsibility is to ensure that contributors know what to expect from us, and 61to arrange that _somebody_ reviews each PR. 62 63We have several different teams that reviewers may be auto-assigned from. You 64should be in teams where you are likely to know who to re-assign a PR to, if you 65can't review it yourself. The teams are determined by the `CODEOWNERS` file at 66the root of the Wasmtime repository. But despite the name, membership in these 67teams is _not_ about who is an authority or "owner" in a particular area. So 68rather than creating a team for each fine-grained division in the repository 69such as individual target architectures or WASI extensions, we use a few 70coarse-grained teams: 71 72- [wasmtime-compiler-reviewers][]: Cranelift and Winch 73- [wasmtime-core-reviewers][]: Wasmtime, including WASI 74- [wasmtime-fuzz-reviewers][]: Fuzz testing targets 75- [wasmtime-default-reviewers][]: Anything else, including CI and documentation 76 77[wasmtime-compiler-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-compiler-reviewers 78[wasmtime-core-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-core-reviewers 79[wasmtime-fuzz-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-fuzz-reviewers 80[wasmtime-default-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-default-reviewers 81 82Ideally, auto-assigned reviewers should be attending the regular Wasmtime or 83Cranelift meetings, as appropriate for the areas they're reviewing. This is to 84help these reviewers stay aware of who is working on what, to more easily hand 85off PRs to the most relevant reviewer for the work. However, this is only 86advice, not a hard requirement. 87 88If you are not sure who to hand off a PR review to, you can look at GitHub's 89suggestions for reviewers, or look at `git log` for the paths that the PR 90affects. You can also just ask other Core Team members for advice. 91 92## General advice 93 94This is a collection of general advice for people who are reviewing pull 95requests. Feel free to take any that you find works for you and ignore the rest. 96You can also open pull requests to suggest more references for this section. 97 98[The Gentle Art of Patch Review][gentle-review] suggests a "Three-Phase 99Contribution Review" process: 100 101[gentle-review]: https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ 102 1031. Is the idea behind the contribution sound? 1042. Is the contribution architected correctly? 1053. Is the contribution polished? 106 107Phase one should be a quick check for whether the pull request should move 108forward at all, or needs a significantly different approach. If it needs 109significant changes or is not going to be accepted, there's no point reviewing 110in detail until those issues are addressed. 111 112On the other end, it's a good idea to defer reviewing for typos or bikeshedding 113about variable names until phase three. If there need to be significant 114structural changes, entire paragraphs or functions might disappear, and then any 115minor errors that were in them won't matter. 116 117The full essay has much more advice and is recommended reading. 118