On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy
<yebbl...@nospamgmail.com> wrote:
"Steven Schveighoffer" <schvei...@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
