Is cross-reviewing allowed?

[Simon King]

Hi!

I read at http://www.sagemath.org/doc/developer/trac.html#reviewing-patches:
"If someone (other than you) has posted a patch for a ticket on the
trac server, you can review it!"

Does that mean, if person A (say, Johan Sebastian Rosenkilde Nielsen)
has a patch on trac (say, at #9976), and person B (say, I) posts
another patch on top of the first, would it be acceptable that person
A reviews person B's patch and vice versa? Or should the two find a
third person for review?

Best regards,
Simon

[Rob Beezer]

Hi Simon,

I believe I wrote that, and it has been my experience that a "reviewer
patch" is often reviewed by the "main author."

As in a reviewer saying to the principal author "I like your patch,
and am ready to give it a positive review, but have posted a patch
with some changes. If you agree with the changes, go ahead and mark
the ticket as positive review."

Dragging John Palmieri into the discussion, see a recent example at:
http://trac.sagemath.org/sage_trac/ticket/10470

There was also a recent proposal about having a group approach to big
complicated tickets, where maybe there'd be some similar cross-
pollination, I'd imagine.

I shouldn't be in the business of making policy just because I wrote
an introduction to help newcomers to Sage development. So if you
think what I wrote went too far, was too exuberant, or others think
this practice is unwise, I'm happy to reflect community practice in
the (oft-delayed) update to this introduction that I've been plotting
for the past few weeks.

As in many things, my personal feeling is that common sense and good
judgement should trump strict rules.

Thanks for bringing this up.

Rob

On Apr 6, 11:21 pm, Simon King <simon.k...@uni-jena.de> wrote:
> Hi!
>
> I read athttp://www.sagemath.org/doc/developer/trac.html#reviewing-patches:

[Simon King]

Hi Rob,

On 7 Apr., 09:09, Rob Beezer <goo...@beezer.cotse.net> wrote:
> I believe I wrote that, and it has been my experience that a "reviewer
> patch" is often reviewed by the "main author."

I was not talking about a reviewer patch. Both patches are fairly non-
trivial.

> As in many things, my personal feeling is that common sense and good
> judgement should trump strict rules.

Agreed. But my personal common sense says that you can find good
arguments for both answers:

* Johan's and my patches tackle independent issues (although both
together are solving the one problem that the ticket is about) Since
it is independent, cross-reviewing is feasible.
* Johan and I had a fairly intense discussion on trac (IRC might have
been better). So, we'd better not cross-review, since we might be
biased.

And if common sense fails to give a clear answer, a policy or at least
some advice from other developers would be helpful.

Cheers,
Simon

[Robert Bradshaw]

Consider intent of the referee system, namely to:

- verify correctness/find bugs, and
- make sure the code (documentation, etc.) meets Sage's standards.

If your patches are independent enough that you feel confident
accomplishing the above points, I think cross-reviewing is just fine.
Otherwise, find someone else to take a look. Completely external
reviewers (if there is even such a thing) are nice, but often have
high (opportunity) cost at diminishing returns.

- Robert

[luisfe]

On Apr 7, 10:16 am, Robert Bradshaw <rober...@math.washington.edu>
wrote:

> On Thu, Apr 7, 2011 at 12:23 AM, Simon King <simon.k...@uni-jena.de> wrote:
> > Hi Rob,
>
> > On 7 Apr., 09:09, Rob Beezer <goo...@beezer.cotse.net> wrote:
> >> As in many things, my personal feeling is that common sense and good
> >> judgement should trump strict rules.

+1 I think that this is a matter of common sense. If you feel that you
will be biased towards the other patch just say so and look for a
third reviewer. What I feel is that if two people are sending patches
on the same ticket. These people are the most suitable to make a
review since they are both interested in the ticket and have already
worked on it, so know much better the code/issue in question.

[kcrisman]

On Apr 7, 4:16 am, Robert Bradshaw <rober...@math.washington.edu>
wrote:

Well said.

If Sage had a problem with people reviewing code that was bad - Dave
K., I'm not talking about spkgs here :-) - willy-nilly, that would be
one thing. We seem to have the opposite problem of people who feel
they don't have time to adequately review code even when they think it
is good. That's not really a bad problem to have! But at any rate
cross-reviewing is very common, and I think should be encouraged until
such time as there are enough reviewers for a given area to make it
unnecessary.

- kcrisman