← Back to team overview

openerp-community team mailing list archive

Re: [slightly OT] code review at mozilla

 

Hello,


On Mon, Jan 21, 2013 at 11:11 AM, Niels Huylebroeck <nh@xxxxxxxxxx> wrote:

> Very nice article, well worth the 10 minutes to read.
>
> About the only thing I want to add is that our current process doesn't
> have specific goals of how many reviews should happen before something
> "may" be merged and maybe also we need to ask the first reviewers to
> request specific reviews from other people if they are knowledgeable on the
> subject.
>
> Right now I try to stay out of merge requests which are outside my comfort
> zone because my input there is of limited functional value, I can only
> point out some nit (as the article calls them), whenever I do see some code
> that interacts with code I see/use regularly I give it a more thorough look
> and if I happen to have a functioning database which can easily test the
> modifications in a running instance I will run that too.
>
> Perhaps we should establish the review level as those 3 levels ? Something
> like these types:
> * nit
> * functional
> * practical
>

I am agree with this levels after read article named here and its totally
necessary clean the process with levels and maybe team per level.

At functional level just testing can helps here, follow as strict process
testing if pass test functional reviewer can check it.
In technical everything is clear IMO.

But there is something what we need to take in care is about "the tool" to
do it, LP does not help us so much.

We can read to another code review process from Chrome[1], there is time
define to reviewers, testing; simple and clear process.


Regards,

[1] http://dev.chromium.org/developers/contributing-code.



> We could then ask that a merge proposal would have at least a certain
> amount of reviews on each type, something like 1 practical, 2 functional, 4
> nit reviews.
> The numbers are fictional but indicate a certain scaling value depending
> on the review "value", I'd rather have it that the nit reviews do not
> "grant" access to merge the request.
>
> Further possible approaches (not sure how feasible they are to track the
> requirements) would be that instant merging is allowed given 1 practical or
> 2 functional reviews, if the merge proposal had only 1 functional review
> for more than a week I think it would be safe to integrate the proposal as
> well. This would ensure a smooth flow even if some people happen to have
> some less time to do reviews with their expertise.
>
> I'm all open for more discussion about these numbers and levels ofcourse,
> just consider this a stepping stone.
>
> Regards,
> Niels
>
>
> 2013/1/21 Alexandre Fayolle <alexandre.fayolle@xxxxxxxxxxxxxx>
>
>> I found the following article via Linux Weekly News, and found it very
>> interesting, so I thought I might share it with the community.
>>
>> http://vocamus.net/dave/?p=1569
>>
>> Thanks for your time, and keep on sending MP. We are happy to review
>> them :-)
>>
>> --
>> Alexandre Fayolle
>> Chef de Projet
>> Tel : + 33 (0)4 79 26 57 94
>>
>> Camptocamp France SAS
>> Savoie Technolac, BP 352
>> 73377 Le Bourget du Lac Cedex
>> http://www.camptocamp.com
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openerp-community
>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~openerp-community
>> More help   : https://help.launchpad.net/ListHelp
>>
>
>
>
> --
> Niels Huylebroeck
> Lead Architect   --   Agaplan
> Tel. : +32 (0) 93 95 98 90
> Web : http://www.agaplan.eu
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>
>


-- 
Cristian Salamea
@ovnicraft

References