www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - D pull request review process -- strawman formal definition, query

reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
The review process for D pull requests is basically non-existent.  We need  
to change this.  Without any understanding of how the process works, both  
reviewers and pull requesters are left to their own devices and  
assumptions to determine what is going on (see recent anecdote on a pull  
request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com ).  We  
currently have a nice (informal but defined) process for large library  
changes, and it seems to work well.  We should do the same at a smaller  
scale, for pull requests.

I want to define a process for a requester to follow in order to submit a  
pull request, including pre-review requirements, objective measurements  
for acceptance, and review workflow/schedule.  We also need a way to have  
people generally responsible for reviews.  At the moment, reviews are  
ad-hoc, with no clear schedule or assignment.  This can result in  
misconceptions and bit-rotting of pull requests.  We need to try and  
present a friendly and inviting process so that people actually WANT to  
contribute (see previously mentioned thread).  I realize this is a large  
commitment, but if we don't have some sort of responsibility (myself  
included), we won't do it.  There are a lot of unpulled pull requests, and  
I think this can be a very detrimental/demotivating thing for a  
community-driven project.

To that end, here is a strawman for discussion.  This is for druntime and  
phobos only, I don't know if this will work for dmd or other projects:

1) pull request prerequisites
    a) pull must pass auto tester before being reviewed.  If you submit a  
pull request and it fails, you need to fix it before a review can be  
done.  Exceptions can be made if the auto tester isn't able to test the  
code properly (e.g. you have a pull for phobos and a pull for druntime  
that depend on each other).
    b) pull request description is sufficient to understand the reasoning  
behind the request.
    c) any related bugzilla requests should be identified.  If this fixes a  
bug, the bug MUST be in bugzilla before pull request is reviewed.  It is a  
good idea to make sure this is actually a bug before creating a pull  
request!  Use the forums/newsgroups.
    d) If pull request is for a large feature (e.g. replacement or addition  
of a module), the library change voting process should be followed on the  
newsgroups instead of this process.

2) pull request review process stages
    a) submitted - pull request awaiting initial verification of pull  
request prerequisites.
    b) unassigned - pull request passes prerequisites, and is awaiting  
assignment to an approved reviewer (process for assignment TBD)
    c) ready for review - pull request is assigned, but reviewer has not  
started looking at it.
    d) official review - pull request in review by reviewer.  Next stage  
can be e - h.
    e) needs work - pull request needs work to be able to be accepted (only  
use this stage if original requester is not immediately responsive).  Go  
back to c after fixed.
    f) rejected - pull request is rejected.  Can be re-opened by another  
official contributor.
    g) approved - pull request is approved for pull.
    h) conditionally approved - pull request is approved, but with minor  
changes (e.g. comment or ddoc changes).
    i) pulled/closed - pull request is approved by second reviewer (this  
does not need to be an in-depth review)

3) Objective measurements required for pull.  All pulls must pass these  
requirements, plan accordingly.
    a) Passes auto-tester on all platforms.  Exceptions can be made if the  
auto-tester fails because of a limitation in the auto-tester, but must  
pass unit tests on at least one failed platform if that is the case.
    b) Code is formatted properly (TBD).
    c) 2 reviewers must approve of the feature.  Secondary review does not  
need to be in-depth
    d) License must be boost.  Any copyrights must be CORRECTLY attributed  
to authors in the designated comment area (if you used code from somewhere  
else, you must include the appropriate copyright information, and the  
license must be able to be re-licensed as boost).  Any large port of code  
should include a courtesy link to the original project unless the original  
project's author is OK with not including the link.
    e) Any related bugzilla entries must be referenced BY LINK in the pull  
request, and the bugzilla entries must reference BY LINK the pull  
request.  This is the requester's responsibility to set up.
    f) Any public-facing code must be properly documented for DDoc to  
generate.  At this point, the actual docs don't have to be verified as  
properly building (because it's freaking annoying to build them).  If the  
auto-tester is able to do this at some point, then this would become a  
requirement.

4) Reviewers' roles and responsibilities.  I am not defining how a  
reviewer gets assigned, not sure how that should work, and it likely  
depends on the tool we use.
    a) When reviewer accepts role as primary reviewer, he/she should review  
the pull request or decline the role as soon as possible.
    b) if the role is accepted, the reviewer should review the pull request  
within a reasonable time frame (what is this, a month?  a week?), or at  
least acknowledge how long they think it should take to get to it.   
Feedback is very important so contributors are not discouraged.
    c) once the review has started, for complex pull requests (or issues  
with the reviewer's schedule) that would require more than a few days or  
so to review, the reviewer should note this.
    d) If there are extra volunteer participants during the review, the  
reviewer should moderate the discussion and comment on review criticisms  
that are not clear-cut.  In other words, the reviewer should be the  
official word on whether a criticism is valid and requires a change.  Not  
sure if there should be some sort of appeals system.
    e) Primary reviewer is responsible for ensuring any objective  
measurements are fulfilled that AREN'T automatically handled (we should  
handle as much as possible automatically).
    f) Secondary reviewer does not need to do an in-depth review.  This  
should not take more than 5-10 minutes.
    g) The primary reviewer is responsible for changing pull-request status  
in whatever tool we choose to track this.
    h) One of the reviewers (not sure if it should be primary or secondary)  
should close any bugzilla bugs fixed by the pull request.  Can this be  
automated?

==================

OK, so now to implement this kind of thing, we need to have a  
collaboration tool that allows tracking the review through its workflow.   
No idea what the best tool or what a good tool would be.  We need  
something kind of like an issue tracker (can github issue tracker do  
this?).  I don't have a lot of experience with doing online project  
collaboration except with D.  I like trello, but it seems to have not  
caught on here.

-Steve
May 09 2013
next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 5/9/13, Steven Schveighoffer <schveiguy yahoo.com> wrote:

 I like trello, but it seems to have not caught on here.


I'm wondering whether we could use bugzilla for this. It has an
"assigned" field, but this is usually used for bug assignees. Perhaps
some newer version of bugzilla offers a "reviewer" field of some
sort.. or maybe we can introduce our own fields (e.g. pull status --
pending review, in review, accepted, etc).

The benefit of using bugzilla is that we would have everything located
at a central place.
May 09 2013
prev sibling next sibling parent reply "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message 
news:op.wwt44fvkeav7ka stevens-macbook-pro.local...

 The review process for D pull requests is basically non-existent.  We need 
 to change this.  Without any understanding of how the process works, both 
 reviewers and pull requesters are left to their own devices and 
 assumptions to determine what is going on (see recent anecdote on a pull 
  > request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com ).  We 
 currently have a nice (informal but defined) process for large library 
 changes, and it seems to work well.  We should do the same at a smaller  > 
 scale, for pull requests.


I suggest making the process as easy as possible for reviewers.  If it is 
too difficult, people will avoid doing it.  I speak from experience as both 
a reviewer and reviewee.


    h) One of the reviewers (not sure if it should be primary or secondary) 
 should close any bugzilla bugs fixed by the pull request.  Can this be 
 automated?


This is what we are currently doing (sort of) and I think it would be better 
to have the requester be responsible for closing the original bug.  This 
often requires trying each original test case along with other measure to 
ensure the bug was actually fixed.  Sometimes pull requests are only partial 
fixes and while the requester (hopefully) knows this, the reviewer will have 
to work all this out in addition to looking at the actual changes.


 OK, so now to implement this kind of thing, we need to have a 
 collaboration tool that allows tracking the review through its workflow. 
 No idea what the best tool or what a good tool would be.  We need 
 something kind of like an issue tracker (can github issue tracker do 
 this?).  I don't have a lot of experience with doing online project 
 collaboration except with D.  I like trello, but it seems to have not 
 caught on here.

 -Steve


Ideally this goes on bugzilla or github issues so we don't end up 
fragmenting the information even further.

My main problem is when I find I have time and energy to review some pull 
requests, I never know which ones are ready.  I usually find myself looking 
through them from the front or the back, and rarely get very far.

A big improvement would be a two-state system:
- Ready for review
- Not ready for review

The submitter sets it to ready when they think it's done, and it goes to Not 
Ready when any of the following happens:
- An official reviewer decides it needs more work
- It fails the autotester (ideally except for those intermittent failures)
- The submitter decides it needs more work

A reviewer with some spare time then has a short list of 'ready to go' pull 
requests.  This would help a lot with dmd, I don't know about the other 
projects.

A useful third state would be 'Waiting for official review' which means the 
formal review process for phobos modules and Walter/Andrei approval for dmd 
enhancements.

I assume something this simple can be done with github issues...?
May 09 2013
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy  
<yebblies nospamgmail.com> wrote:


 "Steven Schveighoffer" <schveiguy yahoo.com> wrote in message
 news:op.wwt44fvkeav7ka stevens-macbook-pro.local...

 The review process for D pull requests is basically non-existent.  We  
 need
 to change this.  Without any understanding of how the process works,  
 both
 reviewers and pull requesters are left to their own devices and
 assumptions to determine what is going on (see recent anecdote on a pull
  > request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com  
 ).  We
 currently have a nice (informal but defined) process for large library
 changes, and it seems to work well.  We should do the same at a  
 smaller  >
 scale, for pull requests.


 I suggest making the process as easy as possible for reviewers.  If it is
 too difficult, people will avoid doing it.  I speak from experience as  
 both
 a reviewer and reviewee.


I agree, and some of the stages I laid out can be notational, not official  
statuses.

There are two things I want to accomplish here:

1. Make the process defined and transparent so both reviewers and  
submitters understand what is going on, and what is expected.
2. Make someone own the review.  Without ownership, there will be long  
delays, and that reflects a sense of disapproval or apathy towards the  
submitter or his idea, even if that isn't the case.  I like how the  
"feature" review on the newsgroup has a review manager and he sets a  
deadline.  That works out REALLY well.

The recent thread on the review non-process outlined a story from the  
perspective of the submitter that looked to me very different than what  
actually happened from the reviewer side (reading the comments after the  
fact).


    h) One of the reviewers (not sure if it should be primary or  
 secondary)
 should close any bugzilla bugs fixed by the pull request.  Can this be
 automated?


 This is what we are currently doing (sort of) and I think it would be  
 better
 to have the requester be responsible for closing the original bug.


I think we should push for full automation.  I think it can be done with  
github (and actually, I think it already automatically records the commit  
in bugzilla).

The problem I see with making the submitter do it is, the submitter may  
not be active at the time, or may not care.  The pull request is done, and  
he has his fix, but we need to make sure the bug list is updated properly.


 This
 often requires trying each original test case along with other measure to
 ensure the bug was actually fixed.


Oh!  I completely forgot!  Another requirement for a pull request to be  
accepted:

   g) Any fixed bugs MUST be accompanied by a unit test that fails with the  
current code but passes with the change.

If this is included, the automated tests should cover that.


 Sometimes pull requests are only partial
 fixes and while the requester (hopefully) knows this, the reviewer will  
 have
 to work all this out in addition to looking at the actual changes.


These would be more complex cases.  I think they would be rare (at least  
 from a library side, not sure about dmd).  It's important to note that  
reviewers are volunteers, and if you are not willing to take the time to  
do a full review, you should give it to someone else.

Perhaps the "description" requirement can be detailed to say "if this is  
only a partial fix, that should be noted."


 Ideally this goes on bugzilla or github issues so we don't end up
 fragmenting the information even further.


agreed.  I'm really leaning toward github issues since it already  
integrates with github pull requests (right? never used it before),  
probably involves less work to automate.


 My main problem is when I find I have time and energy to review some pull
 requests, I never know which ones are ready.  I usually find myself  
 looking
 through them from the front or the back, and rarely get very far.

 A big improvement would be a two-state system:
 - Ready for review
 - Not ready for review


I think this is important.  A pull request shouldn't be "Ready" until it's  
also submitted to the tracking system.  The issue should reflect the  
current state.  In other words, you should be searching for unassigned  
issues in the correct states, not pull requests.


 The submitter sets it to ready when they think it's done, and it goes to  
 Not
 Ready when any of the following happens:
 - An official reviewer decides it needs more work
 - It fails the autotester (ideally except for those intermittent  
 failures)
 - The submitter decides it needs more work


I think we are on the same page, my stages don't have to be "real"  
statuses, but it's important to formalize workflow and statuses so both  
sides know what is going on and what is expected.

-Steve
May 09 2013
next sibling parent reply "Idan Arye" <GenericNPC gmail.com> writes:
On Friday, 10 May 2013 at 01:25:16 UTC, Steven Schveighoffer 
wrote:

 On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy 
 <yebblies nospamgmail.com> wrote:


 "Steven Schveighoffer" <schveiguy yahoo.com> wrote in message
 news:op.wwt44fvkeav7ka stevens-macbook-pro.local...

   h) One of the reviewers (not sure if it should be primary 
 or secondary)
 should close any bugzilla bugs fixed by the pull request.  
 Can this be
 automated?


 This is what we are currently doing (sort of) and I think it 
 would be better
 to have the requester be responsible for closing the original 
 bug.


 I think we should push for full automation.  I think it can be 
 done with github (and actually, I think it already 
 automatically records the commit in bugzilla).

 The problem I see with making the submitter do it is, the 
 submitter may not be active at the time, or may not care.  The 
 pull request is done, and he has his fix, but we need to make 
 sure the bug list is updated properly.


According to the description of the Bugzilla service hook on 
GitHub, it can automatically close bugs via pull requests, but 
only since version 4.0 of Bugzilla - and the one on d.puremagic 
is only 3.4.1. I have no idea what it takes to upgrade a Bugzilla 
- but the benefit of automatically closing bugs would probably 
justify it.

Another thing - in order for the Bugzilla service hook to work, 
the commit message must refer to it via the syntax "Ticket 123", 
"Bug 123" or "Tracker item 123" - while most commit messages in 
current the pull requests refer to it via the "Issue 123" syntax. 
I think the new review process should enforce proper reference to 
Bugzilla issues.


Anyways, why keep using Bugzilla instead of GitHub issues?
May 09 2013
parent "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Idan Arye" <GenericNPC gmail.com> wrote in message 
news:eltfkesxpdjgjesrljbm forum.dlang.org...

 According to the description of the Bugzilla service hook on GitHub, it 
 can automatically close bugs via pull requests, but only since version 4.0 
 of Bugzilla - and the one on d.puremagic is only 3.4.1. I have no idea 
 what it takes to upgrade a Bugzilla - but the benefit of automatically 
 closing bugs would probably justify it.


Quite often bug reports need to be reviewed before being closed. (At least 
for dmd)


 Anyways, why keep using Bugzilla instead of GitHub issues?


Last I checked github issues still can't do some basic things like moving 
bugs between components, you can find lists online.  I'm sure they will 
improve this in the future...
May 10 2013
prev sibling parent reply "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message 
news:op.wwugwetleav7ka stevens-macbook-pro.local...

 1. Make the process defined and transparent so both reviewers and 
 submitters understand what is going on, and what is expected.


Sounds good.


 2. Make someone own the review.  Without ownership, there will be long 
 delays, and that reflects a sense of disapproval or apathy towards the 
 submitter or his idea, even if that isn't the case.  I like how the 
 "feature" review on the newsgroup has a review manager and he sets a 
 deadline.  That works out REALLY well.


My impression is that sometimes modules sit around for quite a while waiting 
for a review manager.  How are reviewers going to be assigned and how will 
it be ensured that they don't let if fall through the cracks?


 The recent thread on the review non-process outlined a story from the 
 perspective of the submitter that looked to me very different than what 
 actually happened from the reviewer side (reading the comments after the 
 fact).


Having a written process we can refer people to will hopefully help with 
that, even if it is just what we currently have.


    h) One of the reviewers (not sure if it should be primary or 
 secondary)
 should close any bugzilla bugs fixed by the pull request.  Can this be
 automated?


 This is what we are currently doing (sort of) and I think it would be 
 better
 to have the requester be responsible for closing the original bug.


 I think we should push for full automation.  I think it can be done with 
 github (and actually, I think it already automatically records the commit 
 in bugzilla).

 The problem I see with making the submitter do it is, the submitter may 
 not be active at the time, or may not care.  The pull request is done, and 
 he has his fix, but we need to make sure the bug list is updated properly.


I don't think this is a problem with dmd, as most pull requests come from 
the same small group of people. (I think Kenji has done over 1/3)


 This
 often requires trying each original test case along with other measure to
 ensure the bug was actually fixed.


 Oh!  I completely forgot!  Another requirement for a pull request to be 
 accepted:

   g) Any fixed bugs MUST be accompanied by a unit test that fails with the 
 current code but passes with the change.


Sure, but this does not ensure that the tests have full coverage of the 
problem, or even full coverage of the cases presented in the bug report. 
This probably applies to dmd more than the others where failures can have 
very complex interacting causes.


 If this is included, the automated tests should cover that.


I would love to have a way to add a test case, and have the autotester 
ensure both that it fails with the old version, and works with the new one. 
(or the other way around for fail-compilation cases)  One of my pet projects 
is an automated regression sourcer finder where you can give it a test case 
and it attempts to build with the last N releases.  Anyway, another time.


 Sometimes pull requests are only partial
 fixes and while the requester (hopefully) knows this, the reviewer will 
 have
 to work all this out in addition to looking at the actual changes.


 These would be more complex cases.  I think they would be rare (at least 
 from a library side, not sure about dmd).  It's important to note that 
 reviewers are volunteers, and if you are not willing to take the time to 
 do a full review, you should give it to someone else.


Right, the really common one in dmd is bugs that apply to D1.


 Perhaps the "description" requirement can be detailed to say "if this is 
 only a partial fix, that should be noted."


 Ideally this goes on bugzilla or github issues so we don't end up
 fragmenting the information even further.


 agreed.  I'm really leaning toward github issues since it already 
 integrates with github pull requests (right? never used it before), 
 probably involves less work to automate.


Yeah, I think we can use labels to set the states and it has assignment 
built in.


 My main problem is when I find I have time and energy to review some pull
 requests, I never know which ones are ready.  I usually find myself 
 looking
 through them from the front or the back, and rarely get very far.

 A big improvement would be a two-state system:
 - Ready for review
 - Not ready for review


 I think this is important.  A pull request shouldn't be "Ready" until it's 
 also submitted to the tracking system.  The issue should reflect the 
 current state.  In other words, you should be searching for unassigned 
 issues in the correct states, not pull requests.


If we use github issues and the pulls and issues are linked, this should be 
fine.


 The submitter sets it to ready when they think it's done, and it goes to 
 Not
 Ready when any of the following happens:
 - An official reviewer decides it needs more work
 - It fails the autotester (ideally except for those intermittent 
 failures)
 - The submitter decides it needs more work


 I think we are on the same page, my stages don't have to be "real" 
 statuses, but it's important to formalize workflow and statuses so both 
 sides know what is going on and what is expected.

 -Steve


I would like real statuses because they are searchable.  The wiki is almost 
good enough here, but having them connected to the actual pull requests and 
automatically changed when they fail the test suite would be much much 
better.  Maybe this applies better to dmd where the majority are bugfixes 
instead of enhancements, I don't know.  I think we're agreeing on most of 
it.
May 10 2013
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 10 May 2013 03:05:31 -0400, Daniel Murphy  
<yebblies nospamgmail.com> wrote:


 "Steven Schveighoffer" <schveiguy yahoo.com> wrote in message



 2. Make someone own the review.  Without ownership, there will be long
 delays, and that reflects a sense of disapproval or apathy towards the
 submitter or his idea, even if that isn't the case.  I like how the
 "feature" review on the newsgroup has a review manager and he sets a
 deadline.  That works out REALLY well.


 My impression is that sometimes modules sit around for quite a while  
 waiting
 for a review manager.  How are reviewers going to be assigned and how  
 will
 it be ensured that they don't let if fall through the cracks?


We can't be perfect :)  This is a volunteer army.  If that is what  
happens, that is what happens.  At least we should make the submitter  
aware of that likelihood.

A weekly check of pull requests that are assigned but stale might be a  
good idea.


 Oh!  I completely forgot!  Another requirement for a pull request to be
 accepted:

   g) Any fixed bugs MUST be accompanied by a unit test that fails with  
 the
 current code but passes with the change.


 Sure, but this does not ensure that the tests have full coverage of the
 problem, or even full coverage of the cases presented in the bug report.
 This probably applies to dmd more than the others where failures can have
 very complex interacting causes.


Up to your discretion as a reviewer what you require.  It's impossible to  
formalize this requirement as a documented/objective measure.  I would say  
that 9 times out of 10, the bug report will contain a minimized example  
that causes the issue to occur, that should just be included as a  
unit-test.


 If this is included, the automated tests should cover that.


 I would love to have a way to add a test case, and have the autotester
 ensure both that it fails with the old version, and works with the new  
 one.


I didn't consider that the auto-tester wouldn't test that the test case  
fails on the existing code.  I suppose that would just be up to the  
reviewer whether you wanted to go through the effort of testing with that  
test on the current master branch, or eyeball it and assume it does fail.   
The most important thing is that a unit test is inserted to verify the fix  
actually works, and that a regression is caught immediately.


 agreed.  I'm really leaning toward github issues since it already
 integrates with github pull requests (right? never used it before),
 probably involves less work to automate.


 Yeah, I think we can use labels to set the states and it has assignment
 built in.


That is perfect.  A couple people have mentioned we can do this without  
github issues, the pull requests support labels and assignment.  Though  
mutually exclusive labels might be awkward.  Perhaps we should redefine  
the stages with that in mind.


 I think we are on the same page, my stages don't have to be "real"
 statuses, but it's important to formalize workflow and statuses so both
 sides know what is going on and what is expected.


 I would like real statuses because they are searchable.  The wiki is  
 almost
 good enough here, but having them connected to the actual pull requests  
 and
 automatically changed when they fail the test suite would be much much
 better.  Maybe this applies better to dmd where the majority are bugfixes
 instead of enhancements, I don't know.  I think we're agreeing on most of
 it.


I meant not ALL the stages have to be real statuses :)  We clearly want  
something searchable to see the pull requests you are assigned and which  
ones are ready for review.

I will take a look at github issues and github's pull request features to  
see what level of features we need.

-Steve
May 10 2013
parent "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message 
news:op.wwvee6wleav7ka stevens-macbook-pro.local...

 Sure, but this does not ensure that the tests have full coverage of the
 problem, or even full coverage of the cases presented in the bug report.
 This probably applies to dmd more than the others where failures can have
 very complex interacting causes.


 Up to your discretion as a reviewer what you require.  It's impossible to 
 formalize this requirement as a documented/objective measure.  I would say 
 that 9 times out of 10, the bug report will contain a minimized example 
 that causes the issue to occur, that should just be included as a 
 unit-test.


Ok, I think this might be different for dmd vs phobos.


 I didn't consider that the auto-tester wouldn't test that the test case 
 fails on the existing code.  I suppose that would just be up to the 
 reviewer whether you wanted to go through the effort of testing with that 
 test on the current master branch, or eyeball it and assume it does fail. 
 The most important thing is that a unit test is inserted to verify the fix 
 actually works, and that a regression is caught immediately.


Absolutely agreed, this is fundamental.  After all it isn't really fixed 
unless you can guarantee it _stays_ fixed.  Adding tests also has the nice 
effect of pushing the burden of keeping it working onto the next person to 
modify the code. :)


 That is perfect.  A couple people have mentioned we can do this without 
 github issues, the pull requests support labels and assignment.  Though 
 mutually exclusive labels might be awkward.  Perhaps we should redefine 
 the stages with that in mind.


They support labels and assignment because they automatically get an issue 
created for them.  I think you have to turn on issue support in order to 
actually add labels to the pull requests (or at least I can't find the 
interface).
May 10 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 9 May 2013 at 21:10:55 UTC, Steven Schveighoffer 
wrote:

 4) Reviewers' roles and responsibilities.  I am not defining 
 how a reviewer gets assigned, not sure how that should work, 
 and it likely depends on the tool we use.


I think each Phobos module should have an official maintainer.  
The modules' documentation have "Author" sections, but those 
typically list everyone that have ever made a significant 
contribution to a module, and many of those people have long 
since withdrawn from the D community.

I propose that there be additional "Maintainer" sections which, 
for each module, specify *one* person who has the primary 
responsibility for that module.  If and when that person 
disappears from the D community, she or he must be replaced by 
someone else.

Whenever someone makes a pull request, it should be assigned to 
the maintainer for the module which is most affected by the 
request.  That person also has the primary review responsibility 
for the request, in the manner described by Steve, but may of 
course reassign it to someone else if necessary or appropriate.

For example, I would be happy to be the official maintainer of 
std.complex, std.path and std.process.  I feel a certain 
ownership towards those modules, and I very much want to review 
changes made to them.  Unfortunately, time currently does not 
permit me to scan the forums, Github and Bugzilla every day for 
discussions, pull requests and bug reports pertaining to these 
modules.  If, however, someone would assign them to me, I would 
be automatically notified via e-mail, and then I would definitely 
take the time to deal with it.

There will of course be requests that have a large impact on 
several modules, and there should also be someone that takes care 
of coordinating the reviews of these.

Lars
May 09 2013
prev sibling next sibling parent reply "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 9 May 2013 at 21:10:55 UTC, Steven Schveighoffer 
wrote:

 [...]

 I want to define a process for a requester to follow in order 
 to submit a pull request [...]


I agree with most of what you say, except maybe this:


 2) pull request review process stages
    a) submitted - pull request awaiting initial verification of 
 pull request prerequisites.
    b) unassigned - pull request passes prerequisites, and is 
 awaiting assignment to an approved reviewer (process for 
 assignment TBD)
    c) ready for review - pull request is assigned, but reviewer 
 has not started looking at it.
    d) official review - pull request in review by reviewer.  
 Next stage can be e - h.
    e) needs work - pull request needs work to be able to be 
 accepted (only use this stage if original requester is not 
 immediately responsive).  Go back to c after fixed.
    f) rejected - pull request is rejected.  Can be re-opened by 
 another official contributor.
    g) approved - pull request is approved for pull.
    h) conditionally approved - pull request is approved, but 
 with minor changes (e.g. comment or ddoc changes).
    i) pulled/closed - pull request is approved by second 
 reviewer (this does not need to be an in-depth review)


This seems to require yet another tool besides Bugzilla and 
GitHub, which I think we should avoid.  It's bad enough to have 
information in two places, without scattering it even more.  If 
we can somehow make it work within the current toolset, then I am 
all for it.
May 09 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-05-10 08:57, Lars T. Kyllingstad wrote:


 This seems to require yet another tool besides Bugzilla and GitHub,
 which I think we should avoid.  It's bad enough to have information in
 two places, without scattering it even more.  If we can somehow make it
 work within the current toolset, then I am all for it.


We could use labels on github for this.

-- 
/Jacob Carlborg
May 10 2013
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-05-09 23:10, Steven Schveighoffer wrote:


 OK, so now to implement this kind of thing, we need to have a
 collaboration tool that allows tracking the review through its
 workflow.  No idea what the best tool or what a good tool would be.  We
 need something kind of like an issue tracker (can github issue tracker
 do this?).  I don't have a lot of experience with doing online project
 collaboration except with D.  I like trello, but it seems to have not
 caught on here.


Github can handle this. You can assign people to a pull request and add 
labels to indicate different statuses or whatever you want them to mean. 
At least I think you can add labels to pull requests. I don't currently 
have any open pull request for any of my projects so I cannot test it.

-- 
/Jacob Carlborg
May 10 2013
next sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 10 May 2013 07:00:08 -0400, Jacob Carlborg <doob me.com> wrote:


 On 2013-05-09 23:10, Steven Schveighoffer wrote:


 OK, so now to implement this kind of thing, we need to have a
 collaboration tool that allows tracking the review through its
 workflow.  No idea what the best tool or what a good tool would be.  We
 need something kind of like an issue tracker (can github issue tracker
 do this?).  I don't have a lot of experience with doing online project
 collaboration except with D.  I like trello, but it seems to have not
 caught on here.


 Github can handle this. You can assign people to a pull request and add  
 labels to indicate different statuses or whatever you want them to mean.  
 At least I think you can add labels to pull requests. I don't currently  
 have any open pull request for any of my projects so I cannot test it.


That sounds fine too.  I wasn't aware of that feature.

-Steve
May 10 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-05-10 14:41, Steven Schveighoffer wrote:


 That sounds fine too.  I wasn't aware of that feature.


There are milestones as well, if that is of any use.

-- 
/Jacob Carlborg
May 10 2013
prev sibling parent reply "Jesse Phillips" <Jessekphillips+D gmail.com> writes:
On Friday, 10 May 2013 at 11:00:08 UTC, Jacob Carlborg wrote:

 Github can handle this. You can assign people to a pull request 
 and add labels to indicate different statuses or whatever you 
 want them to mean. At least I think you can add labels to pull 
 requests. I don't currently have any open pull request for any 
 of my projects so I cannot test it.


I believe only issues could have labels applied. Which is fine 
for those using issues since the pull will create an issue.
May 10 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-05-10 20:10, Jesse Phillips wrote:


 I believe only issues could have labels applied. Which is fine for those
 using issues since the pull will create an issue.


But currently issues are disabled.

-- 
/Jacob Carlborg
May 10 2013
prev sibling parent Thomas Koch <thomas koch.ro> writes:
Steven Schveighoffer wrote:

 OK, so now to implement this kind of thing, we need to have a
 collaboration tool that allows tracking the review through its workflow.
 No idea what the best tool or what a good tool would be.  We need
 something kind of like an issue tracker (can github issue tracker do
 this?).  I don't have a lot of experience with doing online project
 collaboration except with D.  I like trello, but it seems to have not
 caught on here.


The best tool for this is gerrit:

- http://en.wikipedia.org/wiki/Gerrit_%28software%29#Notable_users
- Presentation: http://skillsmatter.com/podcast/home/intro-git-gerrit
   I've downloaded and cut just the gerrit part:
   http://koch.ro/temp/gerrit_skillsmatter.mp4
- gerrit vs. github pull requests:
http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation

Gerrit for D:

- Gerrit works on top of Git.
- Gerrit is designed to automatically run unit and style checks even before 
a human reviews the code.
- Gerrit tracks the whole history of a change request across all versions of 
this change. One can see diffs between versions of one particular change 
request.
- Gerrit supports topics to further specify what a change is about

Regards, Thomas Koch
May 11 2013