All,
While spending the past few days/weeks in CodeReview, it has become
abundantly clear to me that we absolutely must get away from this idea
of doing huge refactorings in our working copies and landing them in trunk
without any warning. The examples I'm going to use here are the
RequestContext, Action and Blocking refactors.
We've gotten into a very bad habit recently of doing a whole lot of work in
secret in our respective working copies, and then pushing to trunk without
first talking to the community to discuss our plans. This is a bad idea for a
bunch of reasons.
Firstly, it skips the community feedback process until after your code is
already in trunk. By skipping this process--whether it's a formal RfC, or just
chatting with your peers on IRC--you miss out on the chance to get valuable
feedback on your architectural decisions before they land in trunk. Once code
has landed in trunk it is almost always easier to follow up and continue to
"fix" the code that should've been fully spec'd out before checkin.
Also, the community *must* have the chance to call you crazy and say "don't
check that in, please." Going back to my examples of Actions, had the community
been consulted first I would've raised objections about the decisions made with
Actions (I think they should be moved to special pages and the old action urls
made to redirect for back-compat...rather than solidifying the old and crappy
action interface with a new coat of paint). Looking at RequestContexts, had we
talked about this in an RfC first...we probably could've skipped the whole
member variable vs. accessor debate and the several months of minor cleanups
that's entailed (__get() magic is evil, IMHO)
Secondly, this increases the load on reviewers, myself included. When you land
a huge commit in trunk (like the Block rewrite), it takes *forever* to review
the original commit + half a dozen or more followups. This drains reviewer time
and leads to longer release cycles. I think I speak for everyone when I say this
is bad. Small incremental changes are infinitely easier to review than large
batch changes.
If you need to make huge changes: do them in a branch. It's what I did with the
installer and maintenance rewrites, what Roan and Trevor did with ResourceLoader
and what Brian Wolff did with his metadata improvements. Of course after landing
your branches in trunk there will inevitably be some cleanup required, but it
keeps trunk more stable until the branch merge and makes it easier to back out
if we decide to scrap the feature/rewrite.
I know SVN branches suck. But the alternative is having a constantly unstable
trunk due to alpha code that was committed haphazardly. Nobody wins in that
scenario.
So please...I beg everyone. Discuss your changes first. It doesn't have to be
formal (although formal spec'ing is always useful too!), but even having a
second set of eyes to glance over your ideas before committing never hurts
anyone.
-Chad