doc: update Reviewing section of onboarding doc #8086
Conversation
|
Since this seems like a good place to voice suggestions regarding the process… |
| * 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 |
There was a problem hiding this comment.
Did you feel these two points were redundant/any other specific reason to drop them?
There was a problem hiding this comment.
sadly I'm not sure everyone understands this by default so distilling the why is probably still useful imo
There was a problem hiding this comment.
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.
|
💯 on writing out |
|
|
LGTM ;) |
|
Looks good to me |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
PR-URL; nodejs#8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
|
Landed in b180a5b |
PR-URL; #8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
PR-URL; #8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
PR-URL; #8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
PR-URL; #8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
PR-URL; #8086 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: targos - Michaël Zasso <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]>
Checklist
Affected core subsystem(s)
doc
Description of change