[OpenLayers-Dev] request for review

[Tim Schaub]

Hey-

Christopher Schmidt wrote:
> On Sat, Mar 14, 2009 at 10:13:08PM -0400, Schuyler Erle wrote:
>> On Sat, 2009-03-14 at 19:45 -0600, Tim Schaub wrote:
>>> Hey-
>>>
>>> I'm hoping to get a favor.  I'd like someone to review the latest patch 
>>> for http://trac.openlayers.org/ticket/1951.
>>>
>>> r9022 introduced a regression that breaks bounds.intersectsBounds.
>> It looks like I got overzealous in my attempts to refactor
>> intersectsBounds more readable to ordinary mortals, which was dumb,
>> because fixing #1951 wound up not actually requiring those changes after
>> all.
>>
>> Your patch appears to replace the changes to intersectsBounds in r9022
>> with its original code, and adds a test for the case that my refactoring
>> broke. Thanks for finding and fixing this, Tim. I've approved the patch
>> in Trac -- please commit it.
>>
>>
>>> If it doesn't get attention in the next day or so, I think it would be 
>>> reasonable to roll back the changes from r9022.  If you disagree, please 
>>> speak up (or review the patch).
>> I disagree in principle. I think that submitting a change that fixes the
>> old patch and waiting for review was the right response.
> 
> I disagree with your disagreement in principle; if we commit a change
> that is breaking existing functionality, I'd rather rollback and do a
> new change in trunk than leave things broken for an extended period. (I
> think that 48 hours is reasonably acceptable to wait; a week is not.)  
> 

Yeah, I agree with you both.  Thanks for the quick review.  (And a close 
examination will show that intersectsBounds is a still a bit different 
than it was before - there were some redundant checks in there that I 
think your refactor did a good job in eliminating, it just also happened 
to change the functionality a bit too much.)

Anyway, thanks.

Tim

> I would feel less this way if trunk weren't so far from the most recent
> release, I expect; more reasons to 'rlease early, release often', I
> suppose.
> 
> Regards,


-- 
Tim Schaub
OpenGeo - http://opengeo.org
Expert service straight from the developers.