{"id":533,"date":"2015-11-06T17:03:35","date_gmt":"2015-11-06T17:03:35","guid":{"rendered":"http:\/\/blogs.gentoo.org\/lu_zero\/?p=533"},"modified":"2015-11-06T19:10:53","modified_gmt":"2015-11-06T19:10:53","slug":"reviews","status":"publish","type":"post","link":"https:\/\/blogs.gentoo.org\/lu_zero\/2015\/11\/06\/reviews\/","title":{"rendered":"Reviews"},"content":{"rendered":"

This spurred from some events happening in Gentoo<\/a>, since with the move to git<\/strong> we eventually have more reviews<\/strong> and obviously comments<\/strong> over patches can be acceptable (and accepted) depending on a number of factors.<\/p>\n

This short post is about communicating effectively<\/strong>.<\/p>\n

When reviewing patches<\/h2>\n

No point in pepper coating<\/h3>\n

Do not disparage code or, even worse, people. There is no point in being insulting, you add noise<\/strong> to the signal:<\/p>\n

\nYou are a moron! This is shit has no place here, do not do again something this stupid.\n<\/p><\/blockquote>\n

This is not<\/strong> OK: most people will focus on the insult<\/strong> and the technical argument will be totally lost<\/strong>.<\/p>\n

Keep in mind that you want people doing stuff for the project not run away crying.<\/p>\n

No point in sugar coating<\/h3>\n

Do not downplay stupid mistakes that would crash your application (or wipe an operating system) because you think it would hurt<\/strong> the feelings of the contributor.<\/p>\n

    rm -fR \/usr \/local\/foo\n<\/code><\/pre>\n
Is as silly as you like but the impact is HUGE.<\/p>\n
\n This is a tiny mistake, you should not do that again.\n<\/p><\/blockquote>\n
No, it isn’t tiny<\/strong> it is quite a problem.<\/p>\n
Mistakes happen, the review is there to avoid them hitting people, but a modicum of care is needed:
\nwasting<\/strong> other people’s time is still bad<\/strong>.<\/p>\n
Point the mistake directly by quoting the line<\/h3>\n
And use at most 2-3 lines<\/strong> to explain why it is a problem.
\nIf you can’t better if you fix that part yourself or move<\/strong> the discussion on a more direct media e.g. IRC.<\/p>\n
Be specific<\/h4>\n
\nThis kind of change is not portable, obscures the code and does not fix the overflow issue at hand:
\nThe expression as whole could still overflow.\n<\/p><\/blockquote>\n
Hopefully even the most busy person juggling over 5 different tasks will get it.<\/p>\n
Be direct<\/h4>\n
\nDo not suggest the use of those non-portable functions again anyway.\n<\/p><\/blockquote>\n
No room for interpretation, do not do that<\/strong>.<\/p>\n
Avoid clashes<\/h3>\n
If you and another reviewer disagree, move the discussion on another media, there is NO point in spamming
\nthe review system with countless comments.<\/p>\n
When receiving reviews (or waiting for them)<\/small><\/h2>\n
Everybody makes mistakes<\/h3>\n
YOU<\/strong> included, if the reviewer (or more than one) tells you that your changes are not right, there are good odds you are wrong.<\/p>\n
Conversely, the reviewer can make mistakes<\/strong>. Usually is better to move<\/strong> away from the review system and discuss over emails or IRC.<\/p>\n
Be nice<\/h3>\n
There is no point in being confrontational. If you think the reviewer is making a mistake, politely point it out.<\/p>\n
If the reviewer is not nice, do not use the same tone to fit in<\/em>. Even more if you do not like<\/strong> that kind of tone to begin with.<\/p>\n
Wait before answering<\/h3>\n
Do not update your patch or write a reply as soon as you get a notification of a review, more changes might be needed and maybe other reviewers have additional opinions.<\/p>\n
Be patient<\/h3>\n
If a patch is unanswered, ping it<\/strong> maybe once a week, possibly rebasing it if the world changed meanwhile.<\/p>\n
Keep in mind that most of your interaction is with other people volunteering<\/strong> their free time and not getting anything out of it as well, sometimes the real-life takes priority =)<\/p>\n","protected":false},"excerpt":{"rendered":"
This spurred from some events happening in Gentoo, since with the move to git we eventually have more reviews and obviously comments over patches can be acceptable (and accepted) depending on a number of factors. This short post is about communicating effectively. When reviewing patches No point in pepper coating Do not disparage code or, … Continue reading Reviews<\/span><\/a><\/p>\n","protected":false},"author":10,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"spay_email":"","jetpack_publicize_message":"","jetpack_is_tweetstorm":false,"jetpack_publicize_feature_enabled":true},"categories":[3,14],"tags":[],"jetpack_publicize_connections":[],"jetpack_featured_media_url":"","jetpack_sharing_enabled":true,"jetpack_shortlink":"https:\/\/wp.me\/s1aGWH-reviews","_links":{"self":[{"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/posts\/533"}],"collection":[{"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/users\/10"}],"replies":[{"embeddable":true,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/comments?post=533"}],"version-history":[{"count":11,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/posts\/533\/revisions"}],"predecessor-version":[{"id":548,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/posts\/533\/revisions\/548"}],"wp:attachment":[{"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/media?parent=533"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/categories?post=533"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blogs.gentoo.org\/lu_zero\/wp-json\/wp\/v2\/tags?post=533"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}