[OH-Dev] Reviewing pull requests
Asheesh Laroia
lists at asheesh.org
Mon Jan 13 02:57:43 UTC 2014
Summary: Merge it, and make a follow-up commit tightening it up.
Detailed thoughts:
One of my mental goals for patch review from newcomers is that we
ship their stuff as soon as feasible. There's nothing like having
stuff actually land.
People who contribute to OpenHatch seem overwhelmingly like they are happy
to receive code quality/consistency feedback. They're also often very
willing to handle that feedback in a timely fashion, although sometimes
they need help with questions like "How do I update a GitHub pull request?"
(To that end, we should make sure we document that in an easy to understand
way.)
People who contribute to OpenHatch also often are pushing their skills
to their current limit. That is to say, when there is
cruft/consistency/style stuff that you and I see in a patch, often the
submitter doesn't have the "eye" for that yet, so they don't see the issues
as issues. This is another way of saying, "The patch submitter usually
submits the highest quality stuff they know how."
Given the goal of low latency, my usual flow chart for this is... well it's
in my head. I just made a flow chart for the fun of it (it was extremely
fun). Attached as ASCII art. I made it using asciiflow.com;
http://www.asciiflow.com/#585181832642635082/1872680033 is the link.
I'd be quite curious for your thoughts on this flow chart/state machine.
On Sun, Jan 12, 2014 at 6:40 PM, Asheesh Laroia <lists at asheesh.org> wrote:
> Long story short: Merge it as-is, and then add a follow-up commit, and
> leave a comment indicating all the reasons you made the cleanups you
> did in the follow-up commit.
>
> I'll send a longer note to the list shortly.
>
> On Sun, Jan 12, 2014 at 6:32 PM, John Morrissey <jwm at horde.net> wrote:
> > I'm reviewing a pull request, and the changes have a bunch of places
> where
> > the consistency/cruft/style could be better. Nothing fatal, but a general
> > tightening over the entire patch.
> >
> > What do you usually do in cases like these? If I have the submitter fix
> the
> > patch, I'm going to wind up criticizing (politely) pretty evenly over the
> > whole thing. Maybe accept it as-is and make a follow-up commit myself
> > tightening it up?
> >
> > john
> > --
> > John Morrissey _o /\ ---- __o
> > jwm at horde.net _-< \_ / \ ---- < \,
> > www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
> > _______________________________________________
> > Devel mailing list
> > Devel at lists.openhatch.org
> > http://lists.openhatch.org/mailman/listinfo/devel
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openhatch.org/pipermail/devel/attachments/20140112/e8792e4f/attachment-0001.html>
-------------- next part --------------
+--------------------------------------------------+
| Is the submitter sitting right in front of you? |
| (A presence in online chat, e.g. IRC, counts.)|
+--------------------------------------------------+
+ +++
yes | no
+ | +
| | +
| | |
v | |
+-------------------------------------+ | |
| Ask them to revise the submission, | | |
| after providing detailed review, | | |
| and make sure to help them resubmit | | |
| in case they have questions. | | |
+-----------+-------------------------+ | |
| | |
| | |
+-----------v-----------------------+ | |
| Did they finish revising the | | |
| submission to your satisfaction | | |
| before they left your presence? | | |
| | | |
+-----------------------------------+ | |
+ + | |
yes no | |
+ + | |
| +-----------------+ |
| |
v |
+-----------------------------+ |
| Merge it (and say thanks!) | |
+-----------------------------+ |
|
+-------+
|
+-----------------------v------------------------+
| Do you have a reasonable belief that |
| they will respond to feedback within a |
| short period (e.g., 2-3 days, tops)? You |
| might look at their resonsiveness to past |
| comments to get a sense of that, and/or their |
| general tone on the pull request, e.g. their |
| degree of tiredness/frustratedness vs their |
| degree of eagerness. |
+------+-----------------------------------------+
+ +
yes no
+ +
v +--v------------------------------+
+--------------------------------------+ | Are they an experienced |
| Leave a comment indicating the | | contributor, whose last commits |
| code style issues you have, | | indicate that their concept of |
| preferably pointing to existing | | code style is similar to yours? |
| OpenHatch docs or mailing list | | (For first-timers, choose "no".)|
| posts about the style, and say that | +---------------------------------+
| you hope they'll get a chance to | + +
| make the desired changes, but even | yes no
| if not, you'll be happy to make them | + +
| yourself and merge it on or after |+-----v---------------+ |
| 3 days from now. Leave a note saying || Do like this, but | |
| if they want more than 3, just ask. || "a month" instead of| |
+--------------------------------------+| 3 days. | |
^ | | |
| +-----+---------------+ |
| | v
| | +----------------------------------------+
| | | Thank them for this |
| | | contribution, and leave |
+-----------------------------+ | a detailed review indicating |
| what problems you found, |
| preferably pointing to existing |
| OpenHatch docs or mailing list |
| posts about style; merge it; and |
| leave a note on the pull request |
| indicating you've added a follow- |
| up commit addressing those style |
| points, with a link to the commit ID. |
| |
+----------------------------------------+
More information about the Devel
mailing list