[Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)
marc.mutz at kdab.com
Mon Oct 16 13:27:33 CEST 2017
On 2017-10-13 16:03, Jedrzej Nowacki wrote:
> If you do like that then you are doing it wrong. Review process is
> _not_ based
> on a name / company / sun activity. It is based on the change content.
> best people do mistakes.
I'm entirely with Jedrzej here. Do not +2 if you don't understand the
change - all of it, regardless of who it's from. If you only did a
cursory review, give a +1, at most, and mention that it's incomplete. If
you do not understand the header change you used as an example, ask for
it to be clarified (in the commit message, preferably), since chances
are that other reviewers or later git history users might have the same
problems understanding code.
I would even qualify what Thiago wrote in response: I don't want
Thiago's +2 on a QStringView patch if he didn't understand the code.
Sure, I don't want unreasonable bikeshedding over subjective matters in
"my" code, since the author of the patch should have the last say in
these, but I don't want someone to usher my code through because I
happen to be the principal author of the code.
Another, slightly related aspect, and one of the reasons why I said
Viktor got it the wrong way around is that I prefer cross-company
reviews over intra-company ones, and so should anyone. If you ask a
member of your own organisation to review a patch, there are social
effects at work that will make the review less valueable than one done
by an outsider: unreasonable trust in your colleagues, trying to be
helpful by unblocking a patch, ...
Consequently, I accept a +2 from a KDABian only if I'm
above-the-ordinary-ly sure about the correctness of my code. Otherwise,
I wait for Thiago or Olivier or any at tqc to give a second opinion.
Yes, it's a pain to wait for code review, but office-neighbour reviews
just tend to produce bad code/apis, and often you can do better by
writing a page of commit message _more_ than you usually would to
anticipate and answer questions regarding the code ahead of time.
More information about the Development