Commit process [was Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement]

[Ian Boston <ie...@tfd.co.uk>]

(I am asking these questions to get a common understanding of how we  
work as a community, not because I feel I am right or wrong)

I agree, people caring, and reviewing the code before/after and long  
after commit is 100% what this community should be doing. No argument  
there, what I want to know is how we achieve that... because at the  
moment we have 2 processes for code review and its confusing.

So, neither I nor Ben pushed the patch to the appspot codereview. (not  
least because my google id is not subscribed to shindig-dev and for  
some reason I cant make aliases work properly with google id's)

but the patch was pushed into JIRA and then discussed at some length  
on the list, ('Extension to getPerson for  
0.9',SHINDIG-904,SHINDIG-918,'PersonService precludes filtering'),  
starting on 5 Feb.

IMHO, that was review?

If patch + jira is not going to considered sufficient for code review  
then IMHO we need to make that clear so decisions like the one I made  
don't go un-noticed by those with production implementations until  
they are found in production.

BTW, I was thinking of those who had already implemented the SPI in  
making the decision, but obviously I was not privy to their source code.


I have no preference to how we work, either way is good with me, but I  
don't want to be making decisions totally on my own, just because one  
half of the community is working one way, and the other are working  
another.

Ian

(also IMHO, just because something was reviewed doesn't mean that its  
perfect and cant be made better in the future and always the case with  
anything I write :))



On 20 Feb 2009, at 18:22, Adam Winer wrote:

>>
>> As for the commit: In the past, we have reviewed patches in JIRA,  
>> talked
>> about them and the committed them.
>>
>> Have we changed the process to have all patches go into code review  
>> before
>> being comitted ?
>> I dont remember seeing any conclusions about this on list?
>
>
> No, there isn't any such requirement.  Checking in without a review  
> means
> there's a greater chance of someone griping after the commit, which  
> is a bit
> more expensive for all concerned.  That's what's happening now -  
> which is
> all fine, and perfectly normal.  The fact that people care is a Good  
> Thing
> for the health of the project.

Re: Commit process [was Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement]

[Paul Lindner <pl...@hi5.com>]

I think what happened with this change is completely acceptable.

Given my limited bandwidth I can't review every change as it comes  
in.  I'm sure there are plenty of people in my situation as well.

Given that issues are in JIRA and they're referenced in checkins I'm  
happy.

For largish changes I'm happy to see code review, since that has  
proven successful in improving quality.


On Feb 21, 2009, at 3:23 AM, Ian Boston wrote:

> (I am asking these questions to get a common understanding of how we  
> work as a community, not because I feel I am right or wrong)
>
> I agree, people caring, and reviewing the code before/after and long  
> after commit is 100% what this community should be doing. No  
> argument there, what I want to know is how we achieve that...  
> because at the moment we have 2 processes for code review and its  
> confusing.
>
> So, neither I nor Ben pushed the patch to the appspot codereview.  
> (not least because my google id is not subscribed to shindig-dev and  
> for some reason I cant make aliases work properly with google id's)
>
> but the patch was pushed into JIRA and then discussed at some length  
> on the list, ('Extension to getPerson for  
> 0.9',SHINDIG-904,SHINDIG-918,'PersonService precludes filtering'),  
> starting on 5 Feb.
>
> IMHO, that was review?
>
> If patch + jira is not going to considered sufficient for code  
> review then IMHO we need to make that clear so decisions like the  
> one I made don't go un-noticed by those with production  
> implementations until they are found in production.
>
> BTW, I was thinking of those who had already implemented the SPI in  
> making the decision, but obviously I was not privy to their source  
> code.
>
>
> I have no preference to how we work, either way is good with me, but  
> I don't want to be making decisions totally on my own, just because  
> one half of the community is working one way, and the other are  
> working another.
>
> Ian
>
> (also IMHO, just because something was reviewed doesn't mean that  
> its perfect and cant be made better in the future and always the  
> case with anything I write :))
>
>
>
> On 20 Feb 2009, at 18:22, Adam Winer wrote:
>
>>>
>>> As for the commit: In the past, we have reviewed patches in JIRA,  
>>> talked
>>> about them and the committed them.
>>>
>>> Have we changed the process to have all patches go into code  
>>> review before
>>> being comitted ?
>>> I dont remember seeing any conclusions about this on list?
>>
>>
>> No, there isn't any such requirement.  Checking in without a review  
>> means
>> there's a greater chance of someone griping after the commit, which  
>> is a bit
>> more expensive for all concerned.  That's what's happening now -  
>> which is
>> all fine, and perfectly normal.  The fact that people care is a  
>> Good Thing
>> for the health of the project.
>