I think the point is that once you are done doing inline comments, make a whole-PR comment to signal that you've actually read through everything. This isn't about "no more discussion" as much as "I'm done talking, your turn". Reitveld didnt send any comments until you sent them all. Github makes it a bit harder to know when you've reached a stopping point. (It also makes it a bit hard to start with a "this doesn't look right", and come back and remove it when you realize how things are structured.)
John =:-> On Jun 13, 2014 2:05 AM, "Jesse Meek" <jesse.m...@canonical.com> wrote: > In my experience submitting/reviewing is an ongoing/open discussion > (augmented by IRC, email and hangouts) on the code which ends with an LGTM > or some other agreement by all involved in the review/discussion. I think > embracing and supporting this, somewhat messy, process will yield more > productivity than over mechanising the process. > > I may be proposing particularly messy PRs but in my experience I've always > had follow up PRs and comments which render any marker (other than LGTM) > arbitrary. > > On 13/06/14 01:23, Matthew Williams wrote: > > +1 Rick. > > My opinion is it's just an issue of manners. After filling in comments > inline let them know that you're done so they can start working on it. > Likewise if you start doing a review and have some interruption and are > unable to finish it let them know that there might be more to come. > > It's polite > > Matty > > > On Thu, Jun 12, 2014 at 12:48 PM, Richard Harding < > rick.hard...@canonical.com> wrote: > >> We try to put a main comment on the review. Having inline comments does >> not >> complete the review. So after going through the diff, we go back to the >> main pull request and leave a comment there. >> >> "This looks good but I had a few concerns about how it's tested. Please >> don't forget to update the docs as well. Looking forward to this change." >> >> It kind of wraps it up. >> >> The other thing we often do is ping in irc that the review is :+1: or >> 'comments sent'. >> >> https://github.com/juju/juju-gui/pull/328 is kind of an example >> conversation. >> >> Rick >> >> On Thu, 12 Jun 2014, Ian Booth wrote: >> >> > It's also the same when you are responding to review comments. You want >> to mark >> > them all as Done (or whatever) and have those go out in a batch to let >> the >> > reviewer know they can come back and +1. >> > >> > Surely we're not the only people annoyed by this? I wonder what more >> experienced >> > github users do. Or maybe people know that github sucks for code >> reviews and use >> > gerrit or something else? >> > >> > On 12/06/14 20:38, Horacio Duran wrote: >> > > Hey, I don't know if this bugs everyone or just me but it happens very >> > > often that I am working while people are reviewing my code on gh. >> While >> > > people is reviewing and commenting on the code I keep getting mails >> and the >> > > diff page from the pr keeps changing. To know when its all done and I >> can >> > > finally try to answer/fix all the comments I usually wait until my >> phone >> > > stops ringing madly with mails but I think that we could do better. >> At the >> > > end of the diff page there is a comment box where you can add comments >> > > (where you usually add your $$merge$$ or LGTM) We could add something >> > > there, like "Done" just to let the author know we are done with the >> review >> > > and not just reading a big confusing chunk of code. >> > > What do you people think? >> >> -- >> Juju-dev mailing list >> Juju-dev@lists.ubuntu.com >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/juju-dev >> > > > > > > -- > Juju-dev mailing list > Juju-dev@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev > >
-- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
