Reviews on Github

[Nate Finch]

Reviewboard goes down a couple times a month, usually from lack of disk
space or some other BS.  According to a source knowledgeable with these
matters, the charm was rushed out, and the agent for that machine is down
anyway, so we're kinda just waiting for the other shoe to drop.

As for the process things that Ian mentioned, most of those can be
addressed with a sprinkling of convention.  Marking things as issues could
just be adding :x: to the first line (github even pops up suggestions and
auto-completes), thusly:

[image: :x:]This will cause a race condition

And if you want to indicate you're dropping a suggestion, you can use :-1:
 which gives you a thumbs down:

[image: :-1:] I ran the race detector and it's fine.

It won't give you the cumulative "what's left to fix" at the top of the
page, like reviewboard... but for me, I never directly read that, anyway,
just used it to see if there were zero or non-zero comments left.

As for the inline comments in the code - there's a checkbox to hide them
all.  It's not quite as convenient as the gutter indicators per-comment,
but it's sufficient, I think.

On Wed, Sep 14, 2016 at 6:43 PM Ian Booth <ian.booth at canonical.com> wrote:

>
>
> On 15/09/16 08:22, Rick Harding wrote:
> > I think that the issue is that someone has to maintain the RB and the
> > cost/time spent on that does not seem commensurate with the bonus
> features
> > in my experience.
> >
>
> The maintenance is not that great. We have SSO using github credentials so
> there's really no day to day works IIANM. As a team, we do many, many
> reviews
> per day, and the features I outlined are significant and things I (and I
> assume
> others) rely on. Don't under estimate the value in knowing why a comment
> was
> rejected and being able to properly track that. Or having review comments
> collapsed as a gutter indicated so you can browse the code and still know
> that
> there's a comment there. With github, you can hide the comments but
> there's no
> gutter indicator. All these things add up to a lot.
>
>
> > On Wed, Sep 14, 2016 at 6:13 PM Ian Booth <ian.booth at canonical.com>
> wrote:
> >
> >> One thing review board does better is use gutter indicators so as not to
> >> interrupt the flow of reading the code with huge comment blocks. It also
> >> seems
> >> much better at allowing previous commits with comments to be viewed in
> >> their
> >> entirety. And it allows the reviewer to differentiate between issues and
> >> comments (ie fix this vs take note of this), plus it allows the notion
> of
> >> marking stuff as fixed vs dropped, with a reason for dropping if needed.
> >> So the
> >> github improvements are nice but there's still a large and significant
> gap
> >> that
> >> is yet to be filled. I for one would miss all the features reviewboard
> >> offers.
> >> Unless there's a way of doing the same thing in github that I'm not
> aware
> >> of.
> >>
> >> On 15/09/16 07:22, Tim Penhey wrote:
> >>> I'm +1 if we can remove the extra tools and we don't get email per
> >> comment.
> >>>
> >>> On 15/09/16 08:03, Nate Finch wrote:
> >>>> In case you missed it, Github rolled out a new review process.  It
> >>>> basically works just like reviewboard does, where you start a review,
> >>>> batch up comments, then post the review as a whole, so you don't just
> >>>> write a bunch of disconnected comments (and get one email per review,
> >>>> not per comment).  The only features reviewboard has is the edge case
> >>>> stuff that we rarely use:  like using rbt to post a review from a
> random
> >>>> diff that is not connected directly to a github PR. I think that is
> easy
> >>>> enough to give up in order to get the benefit of not needing an
> entirely
> >>>> separate system to handle reviews.
> >>>>
> >>>> I made a little test review on one PR here, and the UX was almost
> >>>> exactly like working in reviewboard:
> >> https://github.com/juju/juju/pull/6234
> >>>>
> >>>> There may be important edge cases I'm missing, but I think it's worth
> >>>> looking into.
> >>>>
> >>>> -Nate
> >>>>
> >>>>
> >>>
> >>
> >> --
> >> Juju-dev mailing list
> >> Juju-dev at lists.ubuntu.com
> >> Modify settings or unsubscribe at:
> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
> >>
> >
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160915/b3d9ab9f/attachment.html>