Commit 727c24f
doc: update Reviewing section of onboarding doc
PR-URL; #8086
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>1 parent 04515b8 commit 727c24f
1 file changed
Lines changed: 31 additions & 25 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -64,31 +64,37 @@ onboarding session. | |||
| 64 | 64 | * will also come more naturally over time | |
| 65 | 65 | ||
| 66 | 66 | ||
| 67 | - * reviewing: | ||
| 68 | - * primary goal is for the codebase to improve | ||
| 69 | - * secondary (but not far off) is for the person submitting code to succeed | ||
| 70 | - * helps grow the community | ||
| 71 | - * and draws new people into the project | ||
| 72 | - * Review a bit at a time. It is **very important** to not overwhelm newer people. | ||
| 73 | - * it is tempting to micro-optimize / make everything about relative perf, | ||
| 74 | - don't succumb to that temptation. we change v8 a lot more often now, contortions | ||
| 75 | - that are zippy today may be unnecessary in the future | ||
| 76 | - * be aware: your opinion carries a lot of weight! | ||
| 77 | - * nits are fine, but try to avoid stalling the PR | ||
| 78 | - * note that they are nits when you comment | ||
| 79 | - * if they really are stalling nits, fix them yourself on merge (but try to let PR authors know they can fix these) | ||
| 80 | - * improvement doesn't have to come all at once | ||
| 81 | - * minimum wait for comments time | ||
| 82 | - * There is a minimum waiting time which we try to respect for non-trivial changes, so that people who may have important input in such a distributed project are able to respond. | ||
| 83 | - * It may help to set time limits and expectations: | ||
| 84 | - * the collaborators are very distributed so it is unlikely that they will be looking at stuff the same time as you are. | ||
| 85 | - * before merging code: give folks at least one working day to respond: "If no one objects, tomorrow at <time> I'll merge this in." | ||
| 86 | - * please always either specify your timezone, or use UTC time | ||
| 87 | - * set reminders | ||
| 88 | - * check in on the code every once in a while (set reminders!) | ||
| 89 | - * 48 hours for non-trivial changes, and 72 hours on weekends. | ||
| 90 | - * if a PR is abandoned, check if they'd mind if you took it over (especially if it just has nits left) | ||
| 91 | - * you have the power to `LGTM` another collaborator or TSC / CTC members' work | ||
| 67 | + * Reviewing: | ||
| 68 | + * The primary goal is for the codebase to improve. | ||
| 69 | + * Secondary (but not far off) is for the person submitting code to succeed. | ||
| 70 | + A pull request from a new contributor is an opportunity to grow the | ||
| 71 | + community. | ||
| 72 | + * Review a bit at a time. Do not overwhelm new contributors. | ||
| 73 | + * It is tempting to micro-optimize and make everything about relative | ||
| 74 | + performance. Don't succumb to that temptation. We change V8 often. | ||
| 75 | + Techniques that provide improved performance today may be unnecessary in | ||
| 76 | + the future. | ||
| 77 | + * Be aware: Your opinion carries a lot of weight! | ||
| 78 | + * Nits (requests for small changes that are not essential) are fine, but try | ||
| 79 | + to avoid stalling the pull request. | ||
| 80 | + * Note that they are nits when you comment: `Nit: change foo() to bar().` | ||
| 81 | + * If they are stalling the pull request, fix them yourself on merge. | ||
| 82 | + * Minimum wait for comments time | ||
| 83 | + * There is a minimum waiting time which we try to respect for non-trivial | ||
| 84 | + changes, so that people who may have important input in such a | ||
| 85 | + distributed project are able to respond. | ||
| 86 | + * For non-trivial changes, leave the pull request open for at least 48 | ||
| 87 | + hours (72 hours on a weekend). | ||
| 88 | + * If a pull request is abandoned, check if they'd mind if you took it over | ||
| 89 | + (especially if it just has nits left). | ||
| 90 | + * Approving a change | ||
| 91 | + * Collaborators indicate that they have reviewed and approve of the | ||
| 92 | + the changes in a pull request by commenting with `LGTM`, which stands | ||
| 93 | + for "looks good to me". | ||
| 94 | + * You have the power to `LGTM` another collaborator's (including TSC/CTC | ||
| 95 | + members) work. | ||
| 96 | + * You may not `LGTM` your own pull requests. | ||
| 97 | + * You have the power to `LGTM` anyone else's pull requests. | ||
| 92 | 98 | ||
| 93 | 99 | ||
| 94 | 100 | * what belongs in node: | |
0 commit comments