Okay, I'll leave this a few days and then request a Review Board instance so we can experiment with it.
Will be entirely voluntary! If it starts to become useful, we can codify how we expect it to be used. Until that point, we can just send our patches to Review Board and ask for a review on the mailing list. On 23 January 2014 03:30, Mutton, James <[email protected]> wrote: > On 1/22/14 3:07 PM, "Russell Branca" <[email protected]> wrote: > > >>Huge +1 to more review. >> >>Let's also setup some code guidelines for Erlang/Javascript/C/HTML so we >>have an authoritative list of rules to follow to ensure code consistency, >>and similarly, let's get some guidelines for git commits messages as well, >>Tim Pope's article comes to mind: >>http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html. >> >>Whichever review tool we decide to use, we should use it universally. We >>should figure out a way to do github integration so that PR's show up in >>the tool, and then we update the PR with a link to the relevant review >>page. We also need a good way to initiate the analogue of a PR in whatever >>tool we use. One of the complications with the review process today, is >>that we're all accustomed to doing review through github PR's, but we can >>use github PR's for branches who exist solely on the ASF origin. I imagine >>this won't be an issue with Hive or Gerrit, but we should verify that >>we're >>not required to initiate code review from third party origins. >> >>On a side note, one of my personal nits with github PR's, that I hope >>might >>be resolved with one of these tools, is losing comments when the code >>changes. For instance, if I do a review and make 10 comments that each >>represent a line item that needs to be addressed, I would like to see a >>task list on the review indicating there are unfinished items to be >>addressed, basically a way to make a set of checkboxes necessary to >>complete the review. I find it odd that github doesn't do this, even more >>so by the fact that changing code to address comments will often mess with >>the commenting and hide the messages you posted, making it challenging to >>decipher whether the review items have been addressed. This is by no means >>a requirement to using any approach, just hoping that someone knows of a >>good way to approach this problem in whatever tool we use. >> >> >>-Russell >> >> >>On Wed, Jan 22, 2014 at 6:06 AM, Benoit Chesneau >><[email protected]>wrote: >> >>> On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <[email protected]> >>>wrote: >>> >>> > My first comment is: if we want more reviews, let's have more >>>committers. >>> > >>> > We double our committer base in 2013, and the results look like this: >>> > >>> > https://www.ohloh.net/p/couchdb/analyses/latest/languages_summary >>> > >>> > And I see comments like this on StackOverflow: >>> > >>> > "I've recently noticed that Couch DB is back in heavy >>>development." >>> >>> >>> > So I think we should continue to aggressively recruit more committers >>> > to the project in 2014. Excelsior! >>> > >>> >>> Welcoming new committers in the project is certainly a good thing but I >>> think it's orthogonal to the need of more review and team work. We >>>should >>> find a good workflow now while we are still a limited number of >>>committers >>> because It will be harder to enforce anything on a larger team. We >>>should >>> settle on some guidelines now. It will also help us to attract more >>>people >>> IMO. >>> >>> >>> > >>> > However, as for the way we do reviews... >>> > >>> > Infra ticket about Gerrit >>> > https://issues.apache.org/jira/browse/INFRA-2205 >>> > >>> > Effort seems to be mothballed. But I'm sure we could restart it if we >>> > were serious. >>> > >>> > However, we could use this: >>> > >>> > https://reviews.apache.org/groups/hive/ >>> > >>> > It is well integrated with Apache infrastructure already, sends mails >>> > to the mailing list, and so on. >>> > >>> > Happy to request an instance and we can experiment with it if we like. >>> > If it doesn't work out, we stop using it. No harm. Could make it >>> > entirely voluntary until we figure out a workflow that we all like. >>> > >>> > Should I do that? >>> > >>> >>> I thought gerrit was already integrated but any tool that could help us >>>to >>> make the review is OK for me. How hive compares to gerrit? Could we also >>> plug it with github like gerrit [1] ? >>> >>> - benoît >>> >>> [1] https://gerrit.googlesource.com/plugins/github/ (and some others) >>> >>> > >>> > On 20 January 2014 15:30, Nick North <[email protected]> wrote: >>> > > On 20 January 2014 12:26, Dale Harvey <[email protected]> wrote: >>> > > >>> > >> I fully agree, its something I mentioned at the couchdb conf in >>> > vancouver, >>> > >> a review first system encourages contributions and has multiple >>> benefits >>> > >> >>> > >> * At least 2 people look at the code, less likely to push silly >>> > mistakes >>> > >> * Can codify and practice review rules >>> > >> * Its much easier to view the current activity in the project >>> > >> * Can bring up points of discussion before its too late >>> > >> >>> > >> But I think the most important thing is that it removes the burden >>>of >>> > >> responsibility from the committer to the project as a whole, also >>> hugely >>> > >> beneficial is that it makes it particularly obvious when a patch >>>has >>> > reach >>> > >> a stalemate and forces someone to make the call. >>> > >> >>> > >> For reference on PouchDB every committer is trusted to push code, >>> nobody >>> > >> (including myself) pushes their own code to master, it goes in the >>>PR >>> > queue >>> > >> and gets a +1 from any other committer (who will usually push it), >>> thats >>> > >> essentially the same process we use at mozilla and coming to think >>>of >>> it >>> > >> any other project I have worked on, any commiter has the ability to >>> -1 a >>> > >> patch at which point they give a reason and usually some solution >>>gets >>> > >> agreed on >>> > >> >>> > > >>> > > I like this system, with one small quibble about responsibility. I >>> don't >>> > > think it should be seen as removing the burden of responsibility >>>from >>> the >>> > > committer to the project as a whole. It becomes easy then for >>>everyone, >>> > > including the committer, to think someone else will find bugs, and >>> no-one >>> > > puts in enough effort. I'd say it is still primarily the >>>responsibility >>> > of >>> > > the committer to ensure that code is error free, but that at least >>>one >>> > > person who knows that area of the code base should sign off on it. >>>Some >>> > > spreading of responsibility is good, but too much can actually lead >>>to >>> a >>> > > decline in quality. >>> > > >>> > > >>> > > Nick >>> > >>> > >>> > >>> > -- >>> > Noah Slater >>> > https://twitter.com/nslater >>> > >>> > > > +1 to reviews and getting code guidelines. Nice to see that Apache has a > reviewboard instance available (https://reviews.apache.org/groups/ hive > link). I've spent a lot of time using reviewboard, it's got a good UI and > API. Reviews are submitted using a separate command line tool > (post-review) to actually post the review, but it lets you make multiple > updates from feedback and allows others to see the diffs on the same > review so you can see how a change evolves. You can leave comments or put > a "ship-it" tag to +1 the commit, then from a distance it's possible to > see all the reviews outstanding and how many votes, if any, they've > received. It's friendly for non-blocking reviews too, since the code > review process is separate. It also lets you can work on reviews a little > at a time and save them in between instead of having to devote a block of > time to code reviews (I do this sometimes on big changes). > > Garrit wants to gate the source-tree if I remember correctly so is > possibly less friendly with remote repositories. > > </JamesM> > -- Noah Slater https://twitter.com/nslater
