On 1/22/14 3:07 PM, "Russell Branca" <chewbra...@apache.org> 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 ><bchesn...@gmail.com>wrote: > >> On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <nsla...@apache.org> >>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 <nort...@gmail.com> wrote: >> > > On 20 January 2014 12:26, Dale Harvey <d...@arandomurl.com> 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>
