[OH-Dev] Reviewing pull requests

[Asheesh Laroia]

Ouch, that's what I get for using >80 columns. Let me know if it's
unreadable and I can recreate it.


On Sun, Jan 12, 2014 at 6:57 PM, Asheesh Laroia <lists at asheesh.org> wrote:

> 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/ac820842/attachment.html>