[phobos] [D-Programming-Language/phobos] a15e68: Accidentally committed with a bunch of debugging c...

[Jonathan M Davis]

> On 2 May 2011 23:18, Jonathan M Davis <jmdavisProg at gmx.com> wrote:
> >> On Mon, May 2, 2011 at 4:45 PM, Jonathan M Davis 
<jmdavisProg at gmx.com>wrote:
> >> > I'm a bit divided on it. Andrei and Lars were pushing for _all_
> >> > check-ins to
> >> > be reviewed. Part of me thinks that that's overkill and yet there have
> >> > been times that other developers have caught stuff that I likely
> >> > wouldn't have even
> >> > with smaller changes.
> >> > 
> >> > It does seem like overkill to require a pull request for smaller
> >> > changes, particularly if they really don't look like they'd cause a
> >> > problem, but at the
> >> > same time, the extra eyes can be really valuable, even when you don't
> >> > expect
> >> > it.
> >> > 
> >> > The truly minor, non-code stuff - such as updating the changelog -
> >> > shouldn't
> >> > need a pull request, but at this point, I'm inclined to agree with
> >> > Andrei and
> >> > Lars that all (or at least very nearly all) code changes should go
> >> > through pull requests.
> >> 
> >> Actually, one of my big concerns with using pull requests is that I want
> >> near-instant feedback from the auto tester to make sure my stuff works
> >> on all platforms.  It's frustrating to have to wait an indeterminate
> >> amount of time for such feedback.
> > 
> > I definitely understand that, but if we do that, then code isn't getting
> > reviewed. And the reviews on github have definitely been helping improve
> > code.
> > 
> > As far as code quality goes, pull requests are very much a good thing.
> > 
> > The flip-side, of course, is the delays. It's not uncommon for a pull
> > request to sit there for a week or more (in fact, I believe that a pull
> > request that gets pulled in in less than a week would be a rarity at
> > this point). But unless we have more people who are regularly reviewing
> > and commenting on pull requests, I don't know how to fix that.
> > 
> > Ideally, there would be a way to test your code on all platforms _before_
> > creating a pull request for it, but at the moment, short of setting up
> > all of the systems yourself for yourself, there's no way to do that.
> > 
> > Still, on the whole, I think that the pull requests have had a positive
> > influence on code quality, and I think that they're a good thing.
> 
> I have to say, I *totally* disagree with that. We've just three
> completely erroneous pull requests merges in.
> The autotester is currently failing because of it.
> (1) your changes to std.datetime, which shouldn't have been pulled in
> until the CTFE bug was fixed;
> (2) two pull requests moving std.math intrinsics to a new druntime
> file core.math.
> This had been discussed as being as being a bad idea, and yet it got
> pulled in anyway, and it doesn't even compile Phobos, on ANY platform.
> 
> I would say, the evidence shows:
> (1)  the pull request system DEFINITELY slows development down
> dramatically. (2) the pull request system DOES NOT improve quality.
> 
> Obviously, code reviews are good.
> The pull requests seem to be working well for DMD. But for Phobos,
> basically, they suck.

I have both received and given several comments in reviews which have improved 
the code which has been checked into Phobos. In that regard, code quality has 
been improved. That doesn't mean that nothing has gotten through, and in the 
case of something like the recent merge of the changes to std.datetime, that 
wouldn't have happened with svn, because the person checking in the code would 
have been the person who made the changes. But the review process which the 
pull requests have added has had definite benefits.

Now, using pull requests has also definitely slowed down how quickly things 
get checked into the main repository, but they've also allowed for more people 
to more easily submit changes.

Having pull requests doesn't stop Phobos devs from doing foolish things. It 
helps catch things in review, but if the Phobos dev doesn't compile the code 
before merging it into the main repository (which should _always_ be done), 
then we're going to run into problems. And that's what's happened. Code was 
reviewed, which caught some issues, but it wasn't actually compiled by the 
person checking in the code (either that, or they weren't using the latest 
version of the compiler), so code which didn't compile was checked in.

I definitely disagree that pull requests do not improve code quality, because 
I firmly believe that the extra reviews _do_ improve code quality. They do 
not, however, catch everything, and they do not stop Phobos developers from 
making mistakes.

- Jonathan M Davis