summaryrefslogtreecommitdiff
path: root/doc/onboarding.md
diff options
context:
space:
mode:
authorRich Trott <rtrott@gmail.com>2016-08-12 14:34:11 -0700
committerRich Trott <rtrott@gmail.com>2016-08-15 22:08:52 -0700
commitb180a5beca7c13c96342c97d216fc85673c815b9 (patch)
treef6a463e2c2f12e68030744673ce8c24aa38ad80d /doc/onboarding.md
parent24e4488891b6189c52ea803efed5087d31651a5f (diff)
downloadnode-new-b180a5beca7c13c96342c97d216fc85673c815b9.tar.gz
doc: update Reviewing section of onboarding doc
PR-URL; https://github.com/nodejs/node/pull/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>
Diffstat (limited to 'doc/onboarding.md')
-rw-r--r--doc/onboarding.md56
1 files changed, 31 insertions, 25 deletions
diff --git a/doc/onboarding.md b/doc/onboarding.md
index a44d2170bc..991809df1e 100644
--- a/doc/onboarding.md
+++ b/doc/onboarding.md
@@ -64,31 +64,37 @@ onboarding session.
* will also come more naturally over time
- * reviewing:
- * primary goal is for the codebase to improve
- * secondary (but not far off) is for the person submitting code to succeed
- * helps grow the community
- * and draws new people into the project
- * Review a bit at a time. It is **very important** to not overwhelm newer people.
- * it is tempting to micro-optimize / make everything about relative perf,
- don't succumb to that temptation. we change v8 a lot more often now, contortions
- that are zippy today may be unnecessary in the future
- * be aware: your opinion carries a lot of weight!
- * nits are fine, but try to avoid stalling the PR
- * note that they are nits when you comment
- * if they really are stalling nits, fix them yourself on merge (but try to let PR authors know they can fix these)
- * improvement doesn't have to come all at once
- * minimum wait for comments time
- * 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.
- * It may help to set time limits and expectations:
- * the collaborators are very distributed so it is unlikely that they will be looking at stuff the same time as you are.
- * before merging code: give folks at least one working day to respond: "If no one objects, tomorrow at <time> I'll merge this in."
- * please always either specify your timezone, or use UTC time
- * set reminders
- * check in on the code every once in a while (set reminders!)
- * 48 hours for non-trivial changes, and 72 hours on weekends.
- * if a PR is abandoned, check if they'd mind if you took it over (especially if it just has nits left)
- * you have the power to `LGTM` another collaborator or TSC / CTC members' work
+ * Reviewing:
+ * The primary goal is for the codebase to improve.
+ * Secondary (but not far off) is for the person submitting code to succeed.
+ A pull request from a new contributor is an opportunity to grow the
+ community.
+ * Review a bit at a time. Do not overwhelm new contributors.
+ * It is tempting to micro-optimize and make everything about relative
+ performance. Don't succumb to that temptation. We change V8 often.
+ Techniques that provide improved performance today may be unnecessary in
+ the future.
+ * Be aware: Your opinion carries a lot of weight!
+ * Nits (requests for small changes that are not essential) are fine, but try
+ to avoid stalling the pull request.
+ * Note that they are nits when you comment: `Nit: change foo() to bar().`
+ * If they are stalling the pull request, fix them yourself on merge.
+ * Minimum wait for comments time
+ * 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.
+ * For non-trivial changes, leave the pull request open for at least 48
+ hours (72 hours on a weekend).
+ * If a pull request is abandoned, check if they'd mind if you took it over
+ (especially if it just has nits left).
+ * Approving a change
+ * Collaborators indicate that they have reviewed and approve of the
+ the changes in a pull request by commenting with `LGTM`, which stands
+ for "looks good to me".
+ * You have the power to `LGTM` another collaborator's (including TSC/CTC
+ members) work.
+ * You may not `LGTM` your own pull requests.
+ * You have the power to `LGTM` anyone else's pull requests.
* what belongs in node: