You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mahout.apache.org by Grant Ingersoll <gs...@apache.org> on 2011/12/13 17:41:17 UTC
Review Requests
What benefit does the review requests provide over just adding comments on JIRA and people reviewing there? So far, it seems like it is yet another place I need to log in.
Re: Review Requests
Posted by Raphael Cendrillon <ce...@gmail.com>.
I think the path should be /trunk
On Tue, Dec 13, 2011 at 10:30 AM, Dmitriy Lyubimov <dl...@gmail.com>wrote:
> ok i am doing something wrong. when i am trying to upload patch there, it
> says
>
> Something broke! (Error 500)
>
> It appears something broke when you tried to go to here. This is
> either a bug in Review Board or a server configuration error. Please
> report this to your administrator.
>
> i specify diff path as / for repo mahout
>
>
>
> On Tue, Dec 13, 2011 at 9:44 AM, Jake Mannix <ja...@gmail.com>
> wrote:
> > You mean other than a web UI to see the patch, without having to download
> > it, make sure you have a clean checkout to apply it to, then fire it up
> in
> > your IDE, again making sure you have actually caught all the diffs?
> >
> > But yes, threaded comments inline with the code and the ability to easily
> > show differences between patch versions lead to this workflow: patch
> > uploaded, review created. People point out problems line by line, the
> > original poster (or someone else) replies in-line, corrects the patch,
> > uploads it again, and the reviewers can click the "show diffs of patch 2
> > relative to patch 1", and very quickly see their concerns were taken care
> > of, click "ship-it", and its good to go.
> >
> > Once you get in the rhythm of it, it leads to far more careful reviewing
> by
> > more eyeballs, IMO. We use it religously at Twitter.
> >
> > -jake
> >
> > On Dec 13, 2011 8:50 AM, "Dmitriy Lyubimov" <dl...@gmail.com> wrote:
> >
> > I guess the fact that you can select a code fragment you are commenting
> on.
> > But I haven't yet fully learned how to use it.
> >
> > On Dec 13, 2011 8:41 AM, "Grant Ingersoll" <gs...@apache.org> wrote:
> >
> > What benefit does the rev...
>
Re: Review Requests
Posted by Dmitriy Lyubimov <dl...@gmail.com>.
ok i am doing something wrong. when i am trying to upload patch there, it says
Something broke! (Error 500)
It appears something broke when you tried to go to here. This is
either a bug in Review Board or a server configuration error. Please
report this to your administrator.
i specify diff path as / for repo mahout
On Tue, Dec 13, 2011 at 9:44 AM, Jake Mannix <ja...@gmail.com> wrote:
> You mean other than a web UI to see the patch, without having to download
> it, make sure you have a clean checkout to apply it to, then fire it up in
> your IDE, again making sure you have actually caught all the diffs?
>
> But yes, threaded comments inline with the code and the ability to easily
> show differences between patch versions lead to this workflow: patch
> uploaded, review created. People point out problems line by line, the
> original poster (or someone else) replies in-line, corrects the patch,
> uploads it again, and the reviewers can click the "show diffs of patch 2
> relative to patch 1", and very quickly see their concerns were taken care
> of, click "ship-it", and its good to go.
>
> Once you get in the rhythm of it, it leads to far more careful reviewing by
> more eyeballs, IMO. We use it religously at Twitter.
>
> -jake
>
> On Dec 13, 2011 8:50 AM, "Dmitriy Lyubimov" <dl...@gmail.com> wrote:
>
> I guess the fact that you can select a code fragment you are commenting on.
> But I haven't yet fully learned how to use it.
>
> On Dec 13, 2011 8:41 AM, "Grant Ingersoll" <gs...@apache.org> wrote: >
> What benefit does the rev...
Re: Review Requests
Posted by Isabel Drost <is...@apache.org>.
On 14.12.2011 Jake Mannix wrote:
> Exactly. Lowers barrier to entry for joining in the review process.
Having Ted review more patches certainly is a +1 for having review board.
Just two wishes from my side:
I can easily parse all our stuff (comments on JIRA, commits and regular mails)
by reading the mailing lists only. The messages sent by review board right now
have a few flaws: They all come from the same mail address, they don't contain
some context from the original patch before the contents but only a link out to
the review board instance. That makes it hard to follow without actually
clicking through. Is there some configuration option to make them more
meaningful?
Also we should not make it any harder for potential new contributors to submit
patches - submitting stuff to review board should be encouraged but remain
strictly optional.
Looking forward to seeing more code-reviews also from non-committers,
Isabel
PS: If there is anyone close to Berlin who has used review board before and
loves it - I'd happy to get a brief introduction in return for a cup of coffee
and some cake :)
Re: Review Requests
Posted by Jake Mannix <ja...@gmail.com>.
On Tue, Dec 13, 2011 at 4:48 PM, Ted Dunning <te...@gmail.com> wrote:
> Look at how many times in the last two weeks that I have been able to make
> time to comment on review board versus how much code I have looked at
> without it. Without review board, I just can't get to most review requests
> lately and so it is all or nothing in terms of getting code commentary out
> of me.
>
Exactly. Lowers barrier to entry for joining in the review process. If
you're
the sort who's already going to be applying patches all over the place,
running
tests, etc, then, well, Yay! For the other people who just have 5 min while
waiting for something else at work to build or some other test to run on
their
laptop (something taking up 90% of the CPU, which doesn't allow for also
running Mahout tests, for example), here's to letting them add their $0.02
-jake
>
> On Tue, Dec 13, 2011 at 3:04 PM, Jake Mannix <ja...@gmail.com>
> wrote:
>
> > On Tue, Dec 13, 2011 at 10:41 AM, Grant Ingersoll <gsingers@apache.org
> > >wrote:
> >
> > >
> > > On Dec 13, 2011, at 12:44 PM, Jake Mannix wrote:
> > >
> > > > You mean other than a web UI to see the patch, without having to
> > download
> > > > it, make sure you have a clean checkout to apply it to, then fire it
> up
> > > in
> > > > your IDE, again making sure you have actually caught all the diffs?
> > >
> > > Are you saying it automatically applies the patch and runs the tests?
> > Now
> > > that would be useful! If not, what's the time saving other than for
> > quick,
> > > on the run feedback for superficial things? Otherwise, don't you kind
> of
> > > have to do those steps anyway to know the tests pass? Or perhaps you
> > have
> > > a compiler + JUnit built into your brain? Because that's the
> > functionality
> > > that takes the most time and once you've done those steps you can just
> as
> > > well view the diffs in your IDE.
> > >
> >
> > No the point is that you can keep reviewing the code without having run
> the
> > tests, make progress until it looks like you would like to feel it's
> ready,
> > and then only at the last minute you or someone else can run the tests.
> It
> > also lets people who are *not ever* going to download the patch and run
> the
> > tests the ability to easily comment on things which need to be changed in
> > the patch.
> >
> >
> > > Does "ship it" then apply and commit the patch? Or do I still have to
> do
> > > all that stuff above anyway?
> > >
> >
> > No, it doesn't integrate with RCS. Internally at Twitter, we have git
> > hooks set up such that you do "git review publish" which creates a review
> > of your current branch against master, and then once you've gotten the
> > requisite ship-its, "git review submit" which closes the review and
> merges
> > your branch into master and pushes it back to origin.
> >
> >
> > > Still skeptical but willing to be convinced,
> > >
> >
> > So ReviewBoard doesn't do anything magical linking to unit tests or svn
> or
> > mvn. It would be nice, and someday I'm sure someone will hook those. As
> > it is, it's most useful in these contexts as higher visibility. You may
> be
> > the main person reviewing some code, and you've got it in your IDE and
> are
> > making lots of comments, running tests, etc. I, as an interested (but
> not
> > *that* interested) party can watch along and follow what's happening in
> the
> > code, and then even make helpful comments in-line without having to
> really
> > jump in and set up yet another branch directory or git branch and apply
> > patches and be Involved.
> >
> > -jake
> >
>
Re: Review Requests
Posted by Ted Dunning <te...@gmail.com>.
Look at how many times in the last two weeks that I have been able to make
time to comment on review board versus how much code I have looked at
without it. Without review board, I just can't get to most review requests
lately and so it is all or nothing in terms of getting code commentary out
of me.
On Tue, Dec 13, 2011 at 3:04 PM, Jake Mannix <ja...@gmail.com> wrote:
> On Tue, Dec 13, 2011 at 10:41 AM, Grant Ingersoll <gsingers@apache.org
> >wrote:
>
> >
> > On Dec 13, 2011, at 12:44 PM, Jake Mannix wrote:
> >
> > > You mean other than a web UI to see the patch, without having to
> download
> > > it, make sure you have a clean checkout to apply it to, then fire it up
> > in
> > > your IDE, again making sure you have actually caught all the diffs?
> >
> > Are you saying it automatically applies the patch and runs the tests?
> Now
> > that would be useful! If not, what's the time saving other than for
> quick,
> > on the run feedback for superficial things? Otherwise, don't you kind of
> > have to do those steps anyway to know the tests pass? Or perhaps you
> have
> > a compiler + JUnit built into your brain? Because that's the
> functionality
> > that takes the most time and once you've done those steps you can just as
> > well view the diffs in your IDE.
> >
>
> No the point is that you can keep reviewing the code without having run the
> tests, make progress until it looks like you would like to feel it's ready,
> and then only at the last minute you or someone else can run the tests. It
> also lets people who are *not ever* going to download the patch and run the
> tests the ability to easily comment on things which need to be changed in
> the patch.
>
>
> > Does "ship it" then apply and commit the patch? Or do I still have to do
> > all that stuff above anyway?
> >
>
> No, it doesn't integrate with RCS. Internally at Twitter, we have git
> hooks set up such that you do "git review publish" which creates a review
> of your current branch against master, and then once you've gotten the
> requisite ship-its, "git review submit" which closes the review and merges
> your branch into master and pushes it back to origin.
>
>
> > Still skeptical but willing to be convinced,
> >
>
> So ReviewBoard doesn't do anything magical linking to unit tests or svn or
> mvn. It would be nice, and someday I'm sure someone will hook those. As
> it is, it's most useful in these contexts as higher visibility. You may be
> the main person reviewing some code, and you've got it in your IDE and are
> making lots of comments, running tests, etc. I, as an interested (but not
> *that* interested) party can watch along and follow what's happening in the
> code, and then even make helpful comments in-line without having to really
> jump in and set up yet another branch directory or git branch and apply
> patches and be Involved.
>
> -jake
>
Re: Review Requests
Posted by Jake Mannix <ja...@gmail.com>.
On Tue, Dec 13, 2011 at 10:41 AM, Grant Ingersoll <gs...@apache.org>wrote:
>
> On Dec 13, 2011, at 12:44 PM, Jake Mannix wrote:
>
> > You mean other than a web UI to see the patch, without having to download
> > it, make sure you have a clean checkout to apply it to, then fire it up
> in
> > your IDE, again making sure you have actually caught all the diffs?
>
> Are you saying it automatically applies the patch and runs the tests? Now
> that would be useful! If not, what's the time saving other than for quick,
> on the run feedback for superficial things? Otherwise, don't you kind of
> have to do those steps anyway to know the tests pass? Or perhaps you have
> a compiler + JUnit built into your brain? Because that's the functionality
> that takes the most time and once you've done those steps you can just as
> well view the diffs in your IDE.
>
No the point is that you can keep reviewing the code without having run the
tests, make progress until it looks like you would like to feel it's ready,
and then only at the last minute you or someone else can run the tests. It
also lets people who are *not ever* going to download the patch and run the
tests the ability to easily comment on things which need to be changed in
the patch.
> Does "ship it" then apply and commit the patch? Or do I still have to do
> all that stuff above anyway?
>
No, it doesn't integrate with RCS. Internally at Twitter, we have git
hooks set up such that you do "git review publish" which creates a review
of your current branch against master, and then once you've gotten the
requisite ship-its, "git review submit" which closes the review and merges
your branch into master and pushes it back to origin.
> Still skeptical but willing to be convinced,
>
So ReviewBoard doesn't do anything magical linking to unit tests or svn or
mvn. It would be nice, and someday I'm sure someone will hook those. As
it is, it's most useful in these contexts as higher visibility. You may be
the main person reviewing some code, and you've got it in your IDE and are
making lots of comments, running tests, etc. I, as an interested (but not
*that* interested) party can watch along and follow what's happening in the
code, and then even make helpful comments in-line without having to really
jump in and set up yet another branch directory or git branch and apply
patches and be Involved.
-jake
Re: Review Requests
Posted by Grant Ingersoll <gs...@apache.org>.
I will say I like the inline comments
On Dec 13, 2011, at 1:41 PM, Grant Ingersoll wrote:
>
> On Dec 13, 2011, at 12:44 PM, Jake Mannix wrote:
>
>> You mean other than a web UI to see the patch, without having to download
>> it, make sure you have a clean checkout to apply it to, then fire it up in
>> your IDE, again making sure you have actually caught all the diffs?
>
> Are you saying it automatically applies the patch and runs the tests? Now that would be useful! If not, what's the time saving other than for quick, on the run feedback for superficial things? Otherwise, don't you kind of have to do those steps anyway to know the tests pass? Or perhaps you have a compiler + JUnit built into your brain? Because that's the functionality that takes the most time and once you've done those steps you can just as well view the diffs in your IDE.
>
>>
>> But yes, threaded comments inline with the code and the ability to easily
>> show differences between patch versions lead to this workflow: patch
>> uploaded, review created. People point out problems line by line, the
>> original poster (or someone else) replies in-line, corrects the patch,
>> uploads it again, and the reviewers can click the "show diffs of patch 2
>> relative to patch 1", and very quickly see their concerns were taken care
>> of, click "ship-it", and its good to go.
>
> Does "ship it" then apply and commit the patch? Or do I still have to do all that stuff above anyway?
>
> Still skeptical but willing to be convinced,
> Grant
>
Re: Review Requests
Posted by Grant Ingersoll <gs...@apache.org>.
On Dec 13, 2011, at 12:44 PM, Jake Mannix wrote:
> You mean other than a web UI to see the patch, without having to download
> it, make sure you have a clean checkout to apply it to, then fire it up in
> your IDE, again making sure you have actually caught all the diffs?
Are you saying it automatically applies the patch and runs the tests? Now that would be useful! If not, what's the time saving other than for quick, on the run feedback for superficial things? Otherwise, don't you kind of have to do those steps anyway to know the tests pass? Or perhaps you have a compiler + JUnit built into your brain? Because that's the functionality that takes the most time and once you've done those steps you can just as well view the diffs in your IDE.
>
> But yes, threaded comments inline with the code and the ability to easily
> show differences between patch versions lead to this workflow: patch
> uploaded, review created. People point out problems line by line, the
> original poster (or someone else) replies in-line, corrects the patch,
> uploads it again, and the reviewers can click the "show diffs of patch 2
> relative to patch 1", and very quickly see their concerns were taken care
> of, click "ship-it", and its good to go.
Does "ship it" then apply and commit the patch? Or do I still have to do all that stuff above anyway?
Still skeptical but willing to be convinced,
Grant
Re: Review Requests
Posted by Jake Mannix <ja...@gmail.com>.
You mean other than a web UI to see the patch, without having to download
it, make sure you have a clean checkout to apply it to, then fire it up in
your IDE, again making sure you have actually caught all the diffs?
But yes, threaded comments inline with the code and the ability to easily
show differences between patch versions lead to this workflow: patch
uploaded, review created. People point out problems line by line, the
original poster (or someone else) replies in-line, corrects the patch,
uploads it again, and the reviewers can click the "show diffs of patch 2
relative to patch 1", and very quickly see their concerns were taken care
of, click "ship-it", and its good to go.
Once you get in the rhythm of it, it leads to far more careful reviewing by
more eyeballs, IMO. We use it religously at Twitter.
-jake
On Dec 13, 2011 8:50 AM, "Dmitriy Lyubimov" <dl...@gmail.com> wrote:
I guess the fact that you can select a code fragment you are commenting on.
But I haven't yet fully learned how to use it.
On Dec 13, 2011 8:41 AM, "Grant Ingersoll" <gs...@apache.org> wrote: >
What benefit does the rev...
Re: Review Requests
Posted by Dmitriy Lyubimov <dl...@gmail.com>.
I guess the fact that you can select a code fragment you are commenting on.
But I haven't yet fully learned how to use it.
On Dec 13, 2011 8:41 AM, "Grant Ingersoll" <gs...@apache.org> wrote:
> What benefit does the review requests provide over just adding comments on
> JIRA and people reviewing there? So far, it seems like it is yet another
> place I need to log in.
>
>