I think that as long as we make it clear what kind of review we're asking for, and make sure that we really do follow up on review comments, that we can have the best of both worlds. I do think that reviewing once all the code is done is kind of fruitless... there's a lot more impetus to just leave the code as it is, since so much time has been invested in it, and there's often just so much code that really reviewing it would take all day if not multiple days.
Reviewing in chunks is a lot more reasonable, it just takes some awareness that you can't expect everything to be perfect and finished (if you want perfect and finished, wait til it's a 16000 line code drop from the feature branch). But it definitely takes discipline on the developer's part to ensure that they actually go back and address whatever review comments were left, even if they can't get to everything before they merge to the feature branch. On Mon, Dec 14, 2015 at 8:22 AM roger peppe <roger.pe...@canonical.com> wrote: > On 14 December 2015 at 15:39, Katherine Cox-Buday > <katherine.cox-bu...@canonical.com> wrote: > > Hey All, > > > > I think we have a mis-alignment in how we currently do reviews and > feature > > branches. > > > > We've switched over to feature-branches which is great and has allowed > > Moonstone to land "good enough" code into our feature branch to support a > > bi-weekly demo and solicit feedback. At the same time, I feel like we're > > wasting people's time asking for a +1 on code that is not intended to be > > landed into master. Often this code will take shortcuts or stub out some > > code, and as the lead I'll make a judgment call to circle-back later. > > Reviewers don't necessarily know this. > > > > Conversely, when we go to land the feature branch into master, these PRs > are > > generally rubber-stamped. > > > > I feel like maybe we have this backwards? > > I feel your pain. > > But the other side of this coin is that feature branches can be very big, > so it's very difficult to give them a comprehensive review when merging > them into master. > > For example, this feature branch (https://github.com/juju/juju/pull/3590) > involved more than 16000 lines of changes ("diff" prints 30616 lines). > I wouldn't have any trust in the results of a code review of all that > code at once. > > I think that if you're going to implement features that you don't want > comprehensively reviewed, it's best to treat them as "spikes" and keep > them out of the juju repo entirely, then try to land them in manageable > chunks on the feature branch (or master). > > In my view, an important attribute of the feature branches is that they > are solidly reviewed and thus OK to land without full review of the entire > diff. > > That said, I believe that it is *also* important that people do try > to review feature branches when they merge, if at all reasonable. It's > often the first time that people not directly involved with the feature > branch actually look at the implications for master. > > cheers, > rog. > > -- > 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
