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

[Antranig Basman]

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