Confirmed users, Bureaucrats and Sysops emeriti
722
edits
User:GavinSharp/Code Review: Difference between revisions
no edit summary
MarkFinkle (talk | contribs) |
GavinSharp (talk | contribs) No edit summary |
||
| (One intermediate revision by the same user not shown) | |||
| Line 6: | Line 6: | ||
** is someone else better suited? | ** is someone else better suited? | ||
** should other people at least be notified/involved? | ** should other people at least be notified/involved? | ||
** is my current queue | ** is my current queue too big? | ||
** can be hard to not let pride get in the way! | ** can be hard to not let pride get in the way! | ||
* should we do this? | * should we do this? | ||
| Line 40: | Line 40: | ||
* documentation | * documentation | ||
** think of future us! think of what might be obvious now, but not later | ** think of future us! think of what might be obvious now, but not later | ||
** add relevant references; keep in mind permanence of those references | |||
** extra comments never hurt anyone (but don't write a novel!) | |||
* platform specific concerns? | * platform specific concerns? | ||
* grab bag of acronyms: l10n/rtl/a11y/1337 | * grab bag of acronyms: l10n/rtl/a11y/1337 | ||
| Line 48: | Line 50: | ||
* be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!) | * be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!) | ||
* focus on substance, not style; outcomes, not process | * focus on substance, not style; outcomes, not process | ||
* be open-minded; avoid the "NIH" mindset | |||
* ask and encourage others to ask for feedback early. review shouldn't be about "big questions" - those should use a feedback request, or comments, or IRC, etc. | * ask and encourage others to ask for feedback early. review shouldn't be about "big questions" - those should use a feedback request, or comments, or IRC, etc. | ||
* when you get the urge to push back, think hard about whether it's worth it. put yourself in the other person's shoes, try to step back and look at the bigger picture, always look on the bright side of life, and most importantly: avoid using too many clichés | * when you get the urge to push back, think hard about whether it's worth it. put yourself in the other person's shoes, try to step back and look at the bigger picture, always look on the bright side of life, and most importantly: avoid using too many clichés | ||
Code reviewing is (perhaps obviously) all about judgement calls; the cost of accepting an imperfect patch can sometimes be outweighed by the cost of asking someone who has invested a lot of time in a patch to revisit it. | Code reviewing is (perhaps obviously) all about judgement calls; the cost of accepting an imperfect patch can sometimes be outweighed by the cost of asking someone who has invested a lot of time in a patch to revisit it. | ||