On Mon, 2011-05-02 at 13:45 -0700, Jonathan M Davis wrote:
> Though I get the impression that the general level of activity of Phobos developers has dropped off since the switch the git and github. There's still definitely work being done, and I'd have to check the archives to be sure, but I'm pretty sure that several developers who were at least semi-active before have essentially ceased checking in code changes. I don't know if it has anything to do with the source control switch or not, but I definitely get the impression that fewer Phobos developers have been active, which makes it harder to work through pull requests, since there are that many fewer people reviewing them.

I can only speak for myself, but the reason I have been less active lately has nothing to do with git or github.  I am currently in the final stages of my PhD, and for the past few months I've been drowning in work.  (However, I've submitted my thesis for review now, and expect to have more time for D-related stuff -- at least until the defense date approaches...)

Personally, I've found that what little programming I've done has been a lot more enjoyable after the switch to git.


> In any case, Lars created a nascent guide for contributions to Phobos, which we should probably build on and update as necessary:
> 
> http://www.kyllingen.net/guide.html
> 
> So, if you're looking for documentation, that's what we have. Once we're certain that it's what we want, we should probably post it on d-programming- language.org and/or www.digitalmars.com.

I intend to continue work on this guide, but I'd appreciate if people could take a brief look at it and comment, so I know if I'm taking it in the right direction.

-Lars

On Mon, 2011-05-02 at 17:05 -0400, David Simcha 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.

One option would be to commit all changes to a "testing" branch and have the auto tester run off that.  Then, every once in a while, Andrei (or someone else) can merge the testing branch with master, provided the auto tester has succeeded for all platforms.

-Lars

On 5/2/11 11:09 PM, Lars Tandle Kyllingstad wrote:
> I can only speak for myself, but the reason I have been less active lately has nothing to do with git or github.  I am currently in the final stages of my PhD, and for the past few months I've been drowning in work.  (However, I've submitted my thesis for review now, and expect to have more time for D-related stuff -- at least until the defense date approaches...)

Good luck!

> Personally, I've found that what little programming I've done has been a lot more enjoyable after the switch to git.

Same here.

Generally my perception is quite the opposite - from what I can tell the pace of development has been furious ever since we switch to github.


Andrei

> On 5/2/11 11:09 PM, Lars Tandle Kyllingstad wrote:
> > I can only speak for myself, but the reason I have been less active lately has nothing to do with git or github.  I am currently in the final stages of my PhD, and for the past few months I've been drowning in work.  (However, I've submitted my thesis for review now, and expect to have more time for D-related stuff -- at least until the defense date approaches...)
> 
> Good luck!
> 
> > Personally, I've found that what little programming I've done has been a lot more enjoyable after the switch to git.
> 
> Same here.
> 
> Generally my perception is quite the opposite - from what I can tell the pace of development has been furious ever since we switch to github.

Oh, plenty has been going on since we switched to github, and I think that it's a definite improvement, but I get the impression that fewer of the folks who are actually Phobos devs have been committing code. It could be a false impression though. Certainly, the activity on the main newsgroup with proposals for inclusion in Phobos has increased. It just seems like there are fewer Phobos devs actively committing code or creating pull requests. I'd have to check the archives though to see whether that's true or not.

- Jonathan M Davis

On Mon, 2011-05-02 at 23:50 -0700, Jonathan M Davis wrote:
> > On 5/2/11 11:09 PM, Lars Tandle Kyllingstad wrote:
> > > I can only speak for myself, but the reason I have been less active lately has nothing to do with git or github.  I am currently in the final stages of my PhD, and for the past few months I've been drowning in work.  (However, I've submitted my thesis for review now, and expect to have more time for D-related stuff -- at least until the defense date approaches...)
> > 
> > Good luck!
> > 
> > > Personally, I've found that what little programming I've done has been a lot more enjoyable after the switch to git.
> > 
> > Same here.
> > 
> > Generally my perception is quite the opposite - from what I can tell the pace of development has been furious ever since we switch to github.
> 
> Oh, plenty has been going on since we switched to github, and I think that it's a definite improvement, but I get the impression that fewer of the folks who are actually Phobos devs have been committing code. It could be a false impression though. Certainly, the activity on the main newsgroup with proposals for inclusion in Phobos has increased. It just seems like there are fewer Phobos devs actively committing code or creating pull requests. I'd have to check the archives though to see whether that's true or not.

You don't have to check the archives. :)

    https://github.com/D-Programming-Language/phobos/graphs/impact

Also, the ohloh page for Phobos has some nice stats (see 'Code
Analysis', for instance):

    https://www.ohloh.net/p/libphobos

I think we're in good shape.

-Lars

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.

> 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

On 5/3/2011 12:38 AM, Don Clugston wrote:
>
> The pull requests seem to be working well for DMD.

That's because I can run the complete test suite locally, and I usually do for all pull requests.

On 5/3/2011 1:01 AM, Jonathan M Davis wrote:
> They do
> not, however, catch everything, and they do not stop Phobos developers from
> making mistakes.
>

The mistakes, as I see them, is in not running the test suite before checking in Phobos code.

The master branch of Phobos should always be able to run the entire test suite successfully (even if some tests have to be disabled for that to happen). Code that breaks that should be on side branches.

The problem with the test suite not running successfully is it completely stalls development for everyone else, because they cannot test their new code.

On 5/3/11 2:22 AM, Walter Bright wrote:
> The problem with the test suite not running successfully is it completely stalls development for everyone else, because they cannot test their new code.

Actually that's not the case with git. Although the trunk is now broken I ran

git reset --hard <<SHA1>>

for each earlier commits in reverse chronological order (see https://github.com/D-Programming-Language/phobos/commits/master) until I got a working version. What works is:

git reset --hard 9955cc027fb63f868e4c

I am now working on top of that, and will merge later with whatever the latest code will be.

Of course that's not to say we should have a broken trunk...


Andrei