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
wants to merge 2 commits into from
Closed

doc: update Reviewing section of onboarding doc #8086

Changes from all commits
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
56 changes: 31 additions & 25 deletions doc/onboarding.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you feel these two points were redundant/any other specific reason to drop them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly I'm not sure everyone understands this by default so distilling the why is probably still useful imo

Copy link
Member Author

@Trott Trott Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to decide which line to remove because they seemed to be saying the same thing (or at least very similar things) just using different words.

Then I thought the document is probably fine without either of them and removed them both.

I think what those two lines say are implied by the line above them.

But I don't feel that strongly about it and can restore one or the other if anyone feels strongly about it.

* 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:
Expand Down