[development] Reviewing patches and making decisions

[Angela Byron]

On 11/4/08 2:30 PM, Anselm wrote:
> Hello,
>
> Having followed this thread for a while, I decided to do my bit, and start
> reviewing patches. I didn't get very far ; I stopped at the first patch.
>
> Not because of the code - I could understand what the original code did, and
> what the patched code did - but because there are two decisions to be made :
>
> 1. In that case - and I guess this is not uncomon - the old behaviour might be
> a bug, but it might have been by design. I have no way of telling.
>
> 2. Changing the old behaviour to the new one could break old modules that
> implicitely relied on the old behaviour. Again, this is probably not uncomon.
> How do I weight the importance of the patch vs stability of old modules ?
>
> So what do I do next ? I don't have a sufficient overview of that part of
> Drupal to make such decisions myself. Should I mark it as reviewed, and add
> as a note what the potential implications are ? Or is there another process
> to follow in such situations ?
>
> Best,
> Anselm

First of all, YAY!! Thank you for stepping up to fill a gigantic, gaping 
need. :) You rock!!

Catch is right that we generally disregard backwards compatibility if 
the improvement makes Drupal faster, more flexible, and/or easier to 
understand. Since there's no specific issue to look at (probably by 
design), what I would do in this situation and any others like it is 
leave a comment to the effect of:

"I reviewed this patch and upon visual inspection it looks good, and I 
was able to confirm that it does what's expected when applied. However, 
I was left with a couple questions that maybe you could help me with. 
This behaviour change is going to break $thingy in modules; are we sure 
the benefit is worth the cost? Further, it also seems like it might be 
desirable for the old behaviour to work the way it currently is, for 
example $scenario. This patch reverts that behaviour. Could you provide 
more information as to why this is necessary?"

(This assumes there wasn't already an explanation earlier in the issue; 
definitely *don't* ask questions that were already painstakingly 
explained because that makes people cranky. :D)

It's perfectly fine and valid to ask clarifying questions to patch 
creators, and they're normally very happy to answer them since you're 
giving their patch what they desire most: focused attention.

Don't be shy to expose "ignorance" or something. You'd be amazed how 
many "dumb" questions I ask about each and every patch that goes into 
core (just ask the cabal in #drupal), and they went and gave me the keys 
to D7. :P If the patch leaves you with questions (esp. if you can read 
the code fine and it makes sense to you), it's normally a good 
indication that either the issue needs a better description and/or 
(usually and) it requires more inline comments to explain why the 
behaviour is desirable so that we don't switch it again later in the 
future and break BC once more.

Making code understandable and fleshing out functionality and use cases 
is just as vital to patch review as optimizing algorithms, and fresh 
eyes are a strong asset that will bring unique perspectives that the 
rest of us miss.

-Angie