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 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
48 changes: 23 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,29 @@ 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.
* Review a bit at a time. Do not overwhelm new contributors.
* It is tempting to micro-optimize and make everything about relative
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.

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.
* Note that they are nits when you comment: `Nit: change foo() to bar().`
* If they are stalling the PR, 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
(especially if it just has nits left).
* You have the power to `LGTM` another collaborator's (including TSC/CTC
members) work.


* what belongs in node:
Expand Down