Crikey, it's a pull request! (was Re: Pull requests and review on every merge to master)

Mon Nov 14 15:46:41 UTC 2011

Hi Antranig,

I previously sent out an e-mail on why I thought this was a good idea. I'm not 100% sure it made it out, as it was sent when our lists were having technical issues. If you didn't get it, please let me know and I'll dig it up and resend. I basically supported the idea on the grounds that it puts all contributors at an equal level, in terms of getting code in. Those who had been committers, now serve more as trustees of the code base.

As for the reviews not taking place, I think there are a few potential reasons.

Reviewers are too busy
Reviewers think another reviewer will look at it
http://en.wikipedia.org/wiki/Bystander_effect
Reviewers don't feel qualified ( could be related to the previous point )

I think we should start by finding solutions to the second two issues. Lack of time is something that will always creep up, and would most likely be addressed by adding more people. However, if we haven't solved the last two issues, it won't matter how many people we throw at it. I'm open for potential solutions, and to hear what others think might be issues. 

Thanks
Justin

On 2011-11-13, at 1:27 AM, Antranig Basman wrote:

> I've just issued our first pull request in a little while for Cindy and my FLUID-4525 FatPanel simplification work - here is the headline summary from the request:
> 
>> The main goal of this branch was a considerable simplification of the code in "FatPanel" UIOptions - this
>> has slashed 1/3 of the code in FatPanelUIOptions.js leaving behind 240 lines which are a lot more
>> semantic than before. This has required widespread tweaks to framework support for "foreign iframes" -
>> the new strategy is to send only jQuery's code (and direct dependents, such as jQuery UI plugins) into
>> the foreign iframes, and keep all of the Fluid framework code (IoC and components) etc. behind as a
>> single system in the "home iframe". This makes a lot of the workflow in FatPanel very much simpler and
>> returns its structure as a single-rooted component tree to be very similar to that in the other two
>> configurations.
> 
>> The opportunity was also taken to resolve a number of issues with the event system that
>> were discovered in the phase leading up to the Infusion 1.4 release (FLUID-4338, FLUID-4398, FLUID-4337,
>> FLUID-4151, and a much older issue FLUID-4028). This rationalises the system a lot and adds considerable
>> expressiveness in the form of new "boiled listeners" - that is, it is not necessary to create an entire
>> new event for each required signature, each listener may freely adjust the signature that it receives
>> just for itself. Also notable is support for "composite boiled events" which allow an event to be derived
>> ("boiled") from more than one base event. The derived event fires only once one firing is received from
>> each of the base set, and its signature may be boiled over any of the component events.
> 
>> The general improvements to the framework for cross-iframe support are under FLUID-4536 - there have also been
>> general improvements in framework diagnostics. Each event firer in the system is now supplied with a
>> descriptive name that allows the identity of each event firing to be logged reasonably intelligibly. This
>> is a prelude to a more general UI for getting access to IoC diagnostics at runtime. There were also some
>> defensive fixes to the IoC system heading off some errors which have (probably) not yet been observed in
>> cases where components are created as a result of the firing of non-IoC events. The "instantiator" system
>> awaits more comprehensive overhaul as part of the FLUID-4392 additive demands blocks work.
> 
> I thought this was also a great time to respond to Michelle's point below. It's one I agree wholeheartedly with the principle of - but I wanted to air a few concerns about whether we can make it work in practice.
> 
> Having more review is certainly a great thing - but there are two real-world issues which intrude
> i) Our resources as a community to provide review are quite limited - even during the urgent period during our last review cycle, there were some times when reviews were backed up as much as two weeks.
> ii) Many of our "disasters" (Colin will now surely take the opportunity to warn me for the use of unnecessary emotive language : P) that we have run into as potentially dangerous bits of code getting into trunk have IME been disaster of review rather than disasters after lack of review... I have been guilty as well as many others in just not looking closely enough at code I have been given to review to see the latent problems in it...
> 
> So these suggest to me that review is a weapon to be used sparingly - I worry that if we put up everything for review constantly, it may just reduce our vigilance for those cases where review is even more important. Personally I appreciated our "two-phase" model, where the period leading up to a release is more tightly controlled, with smaller changes, more carefully reviewed, leaving the period after a release where more sweeping changes can be made more quickly without so much formality.
> 
> That said, I am extremely excited for us to move to the 100% review model, assuming we can actually make it work economically - in the interests of testing this out, I am putting this FLUID-4525 branch up for pull + review in order to see what happens to it :)
> 
> Cheers,
> A.
> 
> On 14/10/2011 13:22, Michelle D'Souza wrote:
>> Hi everyone,
>> 
>> Now that we have 1.4 out I think it's a good time to reflect on our process and consider if we want to make
>> some changes. We had a very long bug parade this time around which has got us into the habit of working in
>> branches and making pull requests. I really like working in this way and I'm wondering what people think
>> about extending that process to open coding.
>> 
>> On the positive side it will mean that at least two people will have knowledge of code before it gets into
>> the master repo. It should also catch bugs earlier in the process and might cut down on some of the review
>> tasks we end up doing at the end of a release cycle.
>> 
>> On the negative side it means that people with push access to the repo will need to be vigilant about
>> looking at pull requests throughout the release cycle.
>> 
>> I'd love to hear what other people think about this as we enter open coding again.
>> 
>> Thanks,
>> 
>> Michelle
> _______________________________________________________
> fluid-work mailing list - fluid-work at fluidproject.org
> To unsubscribe, change settings or access archives,
> see http://lists.idrc.ocad.ca/mailman/listinfo/fluid-work

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.idrc.ocad.ca/pipermail/fluid-work/attachments/20111114/c172fa4a/attachment.htm>