Re: [mantisbt-dev] Code reviews (was: Missing feedback for commit comments)

[Damien R. <dr...@ma...>]

On 14 January 2014 05:50, Victor Boctor <vb...@gm...> wrote:
 > I suggest that the output of this conversation should be a wiki page with
 > the process guidelines.

+1

 >> So I think that us devs should really listen for notifications on 
github.
 >
 > [Victor] Usage of github fully is critical and can't be considered as
 > optional.

+1

 > [Victor] I think we should use pull requests for features, refactoring,
 > dropping features for sure.  However, short term I think it is even of
 > benefit to use it for bug fixes until we get used to the process and get
 > a sense of what should go in the fast lane vs. not.  Starting off with
 > a fast lane and a subjective evaluation criteria can put us in the same
 > spot we are in today.

I think the definition of a "small" change acceptable for the fast
track process should be something along the lines of:

A single commit affecting only one file with a small number of updated 
lines of code (say, <10) within a single function that is not used 
throughout the codebase, or that does (can) not have any impact on 
system functionality (e.g. PHPdoc update, minor CSS/page layout 
adjustments, label changes, whitespace, documentation...)

 > [Victor] +1 for all of the above.  A pull request should stand on its
 > own as a complete feature, complete bug fix, a good stage in a
 > direction (that means we can ship after this checkin and not have to
 > wait for a follow up checkin, if we have to wait, then the feature is
 > not complete and it should remain on a branch or in a pull request).

Agreed

 >
 >> There should be a sufficient delay to allow for reviews/comments
 >> before merging (say, 1 week)
 > [...]
 >  I don't know if we want have a fixed time window independent of the
 > change size / number of sign-offs received.

My concern here is to avoid reverts because someone did not have the
time to provide feedback fast enough (we're all busy) and got around
to reviewing only after changes were committed.

But I guess in such case the busy developer can also post a quick
update saying "please hold this until ..."

 > I also don't want to have checkins based on pure timeout.

That's fine on principle, but sometimes people just don't respond at
all for whatever reason (vacation, busy at work...) so what to do in
this case ? delay forever ?

 >> Finally, we should allow for a "fast track" process for minor ,
 >> changes improvements and most bug fixes, which should go in as a
 >> direct commit, to avoid unnecessary admin overhead.
 >
 > [Victor] I'm OK with going towards this, but not right away.  Let's
 > go all the way and see how it works.  We will then be in a better
 > place to come up with some good guidelines for when to go with which
 > route

I'd still prefer to have the fast track available right away (with the
necessary guidelines in place of course), but if you really feel
strongly about only having it later on, I'll (reluctantly) play along ;)

Finally, I think it's important to communicate / give notice to the
team, prior to undertaking some important or high-impact code changes,
like the recent discussion on the notifications, to avoid 2 devs
working on the same thing in parrallel and potentially different
directions and to reduce risk and complexity of merges down the line.

D