[development] Reviewing patches and making decisions
drupal-devel at webchick.net
Tue Nov 4 23:32:52 UTC 2008
On 11/4/08 2:30 PM, Anselm wrote:
> 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 ?
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.
More information about the development