Code Review

[jwhitcraft]

Hello,

 

I have been thinking about the problem that ZF ran into right after 1.9.3 was release a few weeks ago and then we had to rush a 1.9.3pl1 into release.  Would using a tool such as Crucible from Atlassian help with this problem?

 

What does everyone else think?

 

Jon Whitcraft Indianapolis Motor Speedway [hidden email] P: [phone] - F: [phone]

 

********************
********************
This E-mail (and attachments) may contain confidential/privileged information intended only for the named addressee(s). If you are not an intended recipient, do not read, copy, disseminate or take any action based on the content of this E-mail. Please notify the sender by reply E-mail and erase this E-mail from your system. Your assistance is appreciated. E-mail transmission may not be secure or error-free. The company is not responsible for any loss/damage arising from any virus transmitted.
********************
********************

[weierophinney]

-- Whitcraft, Jon <[hidden email]> wrote
(on Thursday, 08 October 2009, 03:11 PM -0400):
> I have been thinking about the problem that ZF ran into right after 1.9.3 was
> release a few weeks ago and then we had to rush a 1.9.3pl1 into release.  Would
> using a tool such as Crucible from Atlassian help with this problem?
>
> What does everyone else think?

In this case, it might not have helped.

There were no tests for the original behavior, and it was not documented
in the manual either -- it was only documented in passing within the API
docs.

We may use Crucible in the future (we actually have it setup already),
but I want to explore a few other options as well.

(My personal experience with Crucible has not been great, but I have
hope that it can be configured to work better.)

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/

[jwhitcraft]

Matthew,

Sounds good.  I just wanted to throw the idea out there.  I was just looking around Atlassian's site and noticed that.

Jon Whitcraft
Indianapolis Motor Speedway
[hidden email]
Phone: [phone] :: Fax: [phone]




-----Original Message-----
From: Matthew Weier O'Phinney [mailto:[hidden email]]
Sent: Thursday, October 08, 2009 3:19 PM
To: [hidden email]
Subject: Re: [zf-contributors] Code Review

-- Whitcraft, Jon <[hidden email]> wrote
(on Thursday, 08 October 2009, 03:11 PM -0400):
> I have been thinking about the problem that ZF ran into right after 1.9.3 was
> release a few weeks ago and then we had to rush a 1.9.3pl1 into release.  Would
> using a tool such as Crucible from Atlassian help with this problem?
>
> What does everyone else think?

In this case, it might not have helped.

There were no tests for the original behavior, and it was not documented
in the manual either -- it was only documented in passing within the API
docs.

We may use Crucible in the future (we actually have it setup already),
but I want to explore a few other options as well.

(My personal experience with Crucible has not been great, but I have
hope that it can be configured to work better.)

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/


********************
********************
This E-mail (and attachments) may contain confidential/privileged information intended only for the named addressee(s). If you are not an intended recipient, do not read, copy, disseminate or take any action based on the content of this E-mail. Please notify the sender by reply E-mail and erase this E-mail from your system. Your assistance is appreciated. E-mail transmission may not be secure or error-free. The company is not responsible for any loss/damage arising from any virus transmitted.
********************
********************

[Dolf Schimmel]

Wasn't this the moment we came up with the idea of a community release manager?

Dolf
Aka Freeaqingme

On Thu, Oct 8, 2009 at 9:19 PM, Matthew Weier O'Phinney
<[hidden email]> wrote:

> -- Whitcraft, Jon <[hidden email]> wrote
> (on Thursday, 08 October 2009, 03:11 PM -0400):
>> I have been thinking about the problem that ZF ran into right after 1.9.3 was
>> release a few weeks ago and then we had to rush a 1.9.3pl1 into release.  Would
>> using a tool such as Crucible from Atlassian help with this problem?
>>
>> What does everyone else think?
>
> In this case, it might not have helped.
>
> There were no tests for the original behavior, and it was not documented
> in the manual either -- it was only documented in passing within the API
> docs.
>
> We may use Crucible in the future (we actually have it setup already),
> but I want to explore a few other options as well.
>
> (My personal experience with Crucible has not been great, but I have
> hope that it can be configured to work better.)
>
> --
> Matthew Weier O'Phinney
> Project Lead            | [hidden email]
> Zend Framework          | http://framework.zend.com/
>

[Ralph Schindler-2]

Ok, I'm gonna throw out something I've been thinking for a while.

I think we should have a list of projects, sites, and dedicated
contributors (it can be a google spreadsheet for that matter) that have
no problem keeping their project up to date with the latest ZF branch
version.  What this would allow us to do is say "here is the latest zf
1.9.x, please try it in your project".  Then, we have at least 5 days
where we collect feedback before actually publishing the version.  This
will provide us with plenty enough time to catch any
unknown/undocumented usage case breaks.

Having more time between the actual building of a release package, and
the actual publishing of that release package.  Perhaps we call it a
ReleaseCandidate, but if we do, we at least have to alert the
aforementioned list that we'd like them to try it out on their project.

Call this "QA from the field".

-ralph

Whitcraft, Jon wrote:

> Hello,
>
>  
>
> I have been thinking about the problem that ZF ran into right after
> 1.9.3 was release a few weeks ago and then we had to rush a 1.9.3pl1
> into release.  Would using a tool such as Crucible from Atlassian help
> with this problem?
>
>  
>
> What does everyone else think?
>
>  
>
> http://www.whitcraftconsulting.com/images/php5_zce_logo_new.gif 
> <http://zend.com/zce.php?c=ZEND002524&r=219549271>
>
>
>
> Jon Whitcraft
> Indianapolis Motor Speedway
> [hidden email] <mailto:[hidden email]>
>
> P: [phone] - F: [phone]
>
>
>  
>
>
> ********************
> ********************
> This E-mail (and attachments) may contain confidential/privileged
> information intended only for the named addressee(s). If you are not an
> intended recipient, do not read, copy, disseminate or take any action
> based on the content of this E-mail. Please notify the sender by reply
> E-mail and erase this E-mail from your system. Your assistance is
> appreciated. E-mail transmission may not be secure or error-free. The
> company is not responsible for any loss/damage arising from any virus
> transmitted.
> ********************
> ********************

[Matthew Ratzloff]

(My personal experience with Crucible has not been great, but I have

hope that it can be configured to work better.)

Unfortunately, Crucible is the best of the bunch.  Using it with FishEye is basically a requirement, as is throwing several gigs of RAM at it.  Once you do that, it's not that bad.

-Matt

On Thu, Oct 8, 2009 at 12:19 PM, Matthew Weier O'Phinney <[hidden email]> wrote:

-- Whitcraft, Jon <[hidden email]> wrote
(on Thursday, 08 October 2009, 03:11 PM -0400):

> I have been thinking about the problem that ZF ran into right after 1.9.3 was
> release a few weeks ago and then we had to rush a 1.9.3pl1 into release.  Would
> using a tool such as Crucible from Atlassian help with this problem?
>
> What does everyone else think?

In this case, it might not have helped.

There were no tests for the original behavior, and it was not documented
in the manual either -- it was only documented in passing within the API
docs.

We may use Crucible in the future (we actually have it setup already),
but I want to explore a few other options as well.

(My personal experience with Crucible has not been great, but I have
hope that it can be configured to work better.)

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/

[A.J. Brown-3]

Yes, yes, and yes.  After being trained in a using code review in a
team environment, I don't know how some teams survive without them!
It's the best way to encourage consistency, and is a very good
collaborative learning tool.  As long as they don't get too political,
that is.

On Sat, Oct 10, 2009 at 2:04 PM, Matthew Ratzloff
<[hidden email]> wrote:

>> (My personal experience with Crucible has not been great, but I have
>>
>> hope that it can be configured to work better.)
>
> Unfortunately, Crucible is the best of the bunch.  Using it with FishEye is
> basically a requirement, as is throwing several gigs of RAM at it.  Once you
> do that, it's not that bad.
> -Matt
>
> On Thu, Oct 8, 2009 at 12:19 PM, Matthew Weier O'Phinney <[hidden email]>
> wrote:
>>
>> -- Whitcraft, Jon <[hidden email]> wrote
>> (on Thursday, 08 October 2009, 03:11 PM -0400):
>> > I have been thinking about the problem that ZF ran into right after
>> > 1.9.3 was
>> > release a few weeks ago and then we had to rush a 1.9.3pl1 into release.
>> >  Would
>> > using a tool such as Crucible from Atlassian help with this problem?
>> >
>> > What does everyone else think?
>>
>> In this case, it might not have helped.
>>
>> There were no tests for the original behavior, and it was not documented
>> in the manual either -- it was only documented in passing within the API
>> docs.
>>
>> We may use Crucible in the future (we actually have it setup already),
>> but I want to explore a few other options as well.
>>
>> (My personal experience with Crucible has not been great, but I have
>> hope that it can be configured to work better.)
>>
>> --
>> Matthew Weier O'Phinney
>> Project Lead            | [hidden email]
>> Zend Framework          | http://framework.zend.com/
>
>

--
A.J. Brown
web | http://ajbrown.org
phone | [phone]

[Pieter Kokx]

Sorry, i forgot to hit the 'reply all' button:

In most of my projects, I am already having an SVN external on the
latest ZF branch, and I am quite sure that I'm not the only one doing that.

Best Regards,

Pieter Kokx


Ralph Schindler schreef:

> Ok, I'm gonna throw out something I've been thinking for a while.
>
> I think we should have a list of projects, sites, and dedicated
> contributors (it can be a google spreadsheet for that matter) that
> have no problem keeping their project up to date with the latest ZF
> branch version.  What this would allow us to do is say "here is the
> latest zf 1.9.x, please try it in your project".  Then, we have at
> least 5 days where we collect feedback before actually publishing the
> version.  This will provide us with plenty enough time to catch any
> unknown/undocumented usage case breaks.
>
> Having more time between the actual building of a release package, and
> the actual publishing of that release package.  Perhaps we call it a
> ReleaseCandidate, but if we do, we at least have to alert the
> aforementioned list that we'd like them to try it out on their project.
>
> Call this "QA from the field".
>
> -ralph
>
> Whitcraft, Jon wrote:
>> Hello,
>>
>>  
>>
>> I have been thinking about the problem that ZF ran into right after
>> 1.9.3 was release a few weeks ago and then we had to rush a 1.9.3pl1
>> into release.  Would using a tool such as Crucible from Atlassian
>> help with this problem?
>>
>>  
>>
>> What does everyone else think?
>>
>>  
>>
>> http://www.whitcraftconsulting.com/images/php5_zce_logo_new.gif
>> <http://zend.com/zce.php?c=ZEND002524&r=219549271>
>>
>>    
>>
>> Jon Whitcraft
>> Indianapolis Motor Speedway
>> [hidden email] <mailto:[hidden email]>
>>
>> P: [phone] - F: [phone]
>>
>>
>>  
>>
>>
>> ********************
>> ********************
>> This E-mail (and attachments) may contain confidential/privileged
>> information intended only for the named addressee(s). If you are not
>> an intended recipient, do not read, copy, disseminate or take any
>> action based on the content of this E-mail. Please notify the sender
>> by reply E-mail and erase this E-mail from your system. Your
>> assistance is appreciated. E-mail transmission may not be secure or
>> error-free. The company is not responsible for any loss/damage
>> arising from any virus transmitted.
>> ********************
>> ********************
>
>

[Trevor Johns-2]

On Sat, Oct 10, 2009 at 11:04 AM, Matthew Ratzloff
<[hidden email]> wrote:
>> (My personal experience with Crucible has not been great, but I have
>>
>> hope that it can be configured to work better.)
>
> Unfortunately, Crucible is the best of the bunch.  Using it with FishEye is
> basically a requirement, as is throwing several gigs of RAM at it.  Once you
> do that, it's not that bad.
> -Matt

My personal experience with Crucible has been pretty bad too. The
problem isn't performance, it's the workflow.

Namely:

1. The way we have Crucible setup, code reviews are started so that
everybody on the project is invited to the review, which is totally
impractical. If I just want to invite one person, I need to uncheck an
ungodly number of checkboxes.

2. It's not easy to do pre-commit reviews in Crucible, which I greatly
prefer over post-commit reviews. There is a mechanism to upload a
patch file, but that's done outside of SVN/Git. I don't believe
there's any way to start a code review from my command line.

Personally, the best code review experience I've had is with Gerrit
(review.source.android.com), with Rietveld (codereview.appspot.com)
coming in second place.

That being said, I'd love to have a formal code review policy in place
for ZF. I've had an unofficial policy in place for Zend_Gdata for a
while now, since I work directly with most of the other committers on
that module, and it's worked very well.

--
Trevor Johns

[weierophinney]

-- Trevor Johns <[hidden email]> wrote
(on Monday, 12 October 2009, 12:08 PM -0700):

> On Sat, Oct 10, 2009 at 11:04 AM, Matthew Ratzloff
> <[hidden email]> wrote:
> > > (My personal experience with Crucible has not been great, but I have
> > > hope that it can be configured to work better.)
> >
> > Unfortunately, Crucible is the best of the bunch.  Using it with FishEye is
> > basically a requirement, as is throwing several gigs of RAM at it.  Once you
> > do that, it's not that bad.
> > -Matt
>
> My personal experience with Crucible has been pretty bad too. The
> problem isn't performance, it's the workflow.
>
> Namely:
>
> 1. The way we have Crucible setup, code reviews are started so that
> everybody on the project is invited to the review, which is totally
> impractical. If I just want to invite one person, I need to uncheck an
> ungodly number of checkboxes.

Yes, I ran into this as well. With a project the size of ZF... imagine
the pain...

> 2. It's not easy to do pre-commit reviews in Crucible, which I greatly
> prefer over post-commit reviews. There is a mechanism to upload a
> patch file, but that's done outside of SVN/Git. I don't believe
> there's any way to start a code review from my command line.

Well, the idea here would be for reviewing a trunk commit for
inclusion in the release branch and/or because of potential BC breakage.
I think pre-commit would be a bit unwieldy and introduce too much
overhead and complexity to the project.

The other issue I had with Crucible had to do with how comments were
relayed to the reviewees and peer reviewers: After each comment was
entered, an email was sent. This led to a ton of emails being sent in
many cases, or comments being deleted *after* an email was already sent.
I'd much prefer that we be able to send only when the reviewer has
finished the review.

> Personally, the best code review experience I've had is with Gerrit
> (review.source.android.com), with Rietveld (codereview.appspot.com)
> coming in second place.

I may look at those.

> That being said, I'd love to have a formal code review policy in place
> for ZF. I've had an unofficial policy in place for Zend_Gdata for a
> while now, since I work directly with most of the other committers on
> that module, and it's worked very well.

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/

[till]

On Mon, Oct 12, 2009 at 10:38 PM, Matthew Weier O'Phinney
<[hidden email]> wrote:
> -- Trevor Johns <[hidden email]> wrote
> (...)
>> Personally, the best code review experience I've had is with Gerrit
>> (review.source.android.com), with Rietveld (codereview.appspot.com)
>> coming in second place.
>
> I may look at those.

I remember looking at both and I decided against Rietveld because it's
AppEngine-based and the other is (was?) for GIT-powered projects. I
ended up baking a couple greasemonkey scripts for code review from a
trac install.

But besides -- why is it so hard to find great tools? :-)

Till

[Graham Anderson]

On Monday 12 October 2009 21:08:02 Trevor Johns wrote:
>There is a mechanism to upload a patch file, but that's done outside of
>SVN/Git. I don't believethere's any way to start a code review from my
>command line.
>

Apologies in advance for thread hi-jack.

Trevor, on the subject of Git, first off thanks for making the effort to track
the SVN repo on github. Second, would you please add the standard incubator as
a remote tracking  branch?

Thanks,
Graham

--
“Experience is the name everyone gives to their mistakes.”
 ☘ Oscar Wilde

[Trevor Johns-2]

On Mon, Oct 12, 2009 at 1:38 PM, Matthew Weier O'Phinney
<[hidden email]> wrote:
> I think pre-commit would be a bit unwieldy and introduce too much
> overhead and complexity to the project.

On the project's I've worked on that use pre-commit reviews, I've
found it to be relatively painless.

It does add a bit of delay when submitting new patches, but it can be
done asynchronously (code, upload patch and ask somebody to review it,
work on something else while waiting on the review).

With post-commit, it's easy to forget to have code reviewed, and it's
more difficult to ensure that the comments from the review actually
get applied. It's also not really friendly to developers without a
commit bit, since there's a high overhead to getting any new patch
committed.

Also, if code is only reviewed before merging into a release branch,
anything that's targeted for a major/minor release (and therefore is
never merged) won't get reviewed. This is outside the original scope
of this discussion, but it's probably good to have major changes like
these reviewed too.

> The other issue I had with Crucible had to do with how comments were
> relayed to the reviewees and peer reviewers: After each comment was
> entered, an email was sent.

I forgot about that. That's also incredibly annoying. :)

On Mon, Oct 12, 2009 at 1:48 PM, till <[hidden email]> wrote:
> I remember looking at both and I decided against Rietveld because it's
> AppEngine-based and the other is (was?) for GIT-powered projects. I
> ended up baking a couple greasemonkey scripts for code review from a
> trac install.

Yes, unfortunately both of them do have some issues that would need to
be addressed before using them with ZF.

The problem with Rietveld is not that it runs on App Engine per se,
but rather that App Engine enforces a request timeout. I've run into
cases where uploading very large patches can cause it to hit this
timeout. :(

And yes, Gerrit is Git only. Not sure what it would take to migrate
it. I personally love Git and wish we used it instead of SVN, but
that's a topic for another thread. ;)

--
Trevor Johns

[Trevor Johns-2]

On Mon, Oct 12, 2009 at 2:29 PM, Graham Anderson
<[hidden email]> wrote:
> Trevor, on the subject of Git, first off thanks for making the effort to track
> the SVN repo on github. Second, would you please add the standard incubator as
> a remote tracking  branch?

Ah, yes, I completely forgot about that.

I'll try to get that done sometime after work this week. If you don't
hear back from me by Friday, feel free to ping me and remind me.

--
Trevor Johns