Skip to content

doc: update Reviewing section of onboarding doc #8086

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

Closed
Trott wants to merge 2 commits into from
Closed

doc: update Reviewing section of onboarding doc #8086

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d758815
doc: update Reviewing section of onboarding doc
Trott Aug 12, 2016
8f18c63
squash: apply suggestions
Trott Aug 12, 2016
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
Prev Previous commit
squash: apply suggestions
  • Loading branch information
@Trott
Trott committed Aug 12, 2016
commit 8f18c63167c5a2ea7b6074882ab5538abb455439
18 changes: 13 additions & 5 deletions doc/onboarding.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,34 @@ onboarding session.
* 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 PR.
to avoid stalling the pull request.
* Note that they are nits when you comment: `Nit: change foo() to bar().`
* If they are stalling the PR, fix them yourself on merge.
* 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 PR open for at least 48 hours (72
hours on a weekend).
* If a PR is abandoned, check if they'd mind if you took it over
* 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:
Expand Down