Branch: refs/heads/master
Home:   https://github.com/D-Programming-Language/phobos

Commit: a15e680a986b71281f7704feaf959fe9e7bfc091
    https://github.com/D-Programming-Language/phobos/commit/a15e680a986b71281f7704feaf959fe9e7bfc091
Author: dsimcha <dsimcha at gmail.com>
Date:   2011-05-01 (Sun, 01 May 2011)

Changed paths:
  M std/parallelism.d

Log Message:
-----------
Accidentally committed with a bunch of debugging code.

David -- you may want to make updates through the pull request system. That gives you the opportunity of having one extra pair of eyes look over the code.

Ideally there should be at least two people involved in a change. Unfortunately, it looks like the second person is me rather often.


Andrei

On 5/1/11 8:31 PM, noreply at github.com wrote:
> Branch: refs/heads/master
> Home:   https://github.com/D-Programming-Language/phobos
>
> Commit: a15e680a986b71281f7704feaf959fe9e7bfc091
>      https://github.com/D-Programming-Language/phobos/commit/a15e680a986b71281f7704feaf959fe9e7bfc091
> Author: dsimcha<dsimcha at gmail.com>
> Date:   2011-05-01 (Sun, 01 May 2011)
>
> Changed paths:
>    M std/parallelism.d
>
> Log Message:
> -----------
> Accidentally committed with a bunch of debugging code.
>
>
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

On 5/1/2011 10:51 PM, Andrei Alexandrescu wrote:
> David -- you may want to make updates through the pull request system. That gives you the opportunity of having one extra pair of eyes look over the code.
>
> Ideally there should be at least two people involved in a change. Unfortunately, it looks like the second person is me rather often.
>

I understand the idea that significant commits should be reviewed, but where do we draw the line?  Reviewing even the most trivial commits seems unnecessarily bureaucratic to me.  In the case of the initial commit, std.parallelism had already been in review for weeks.  (To play Devil's Advocate, though, the changes to the make file might have been worth reviewing.)

Guidelines for this would be nice to have.

I'd say definite changes that should go through a reviewed pull request:

* changes from non-phobos developers (obviously)
* changes to files that you do not "own" (even if trivial).? For example, if I were to find a bug in std.parallelism, I shouldn't commit, I should generate a pull request for review.

* changes that alter the design of a module.
* new code (including adding classes/structs/functions to an existing module)


I think everything else is OK to commit directly instead of generating a pull request, including bug fixes (as long as they are not fixed by doing one of the above).


What do you think?? A list like this (and actually, our entire process should be documented, including how to get included as a phobos developer) should be posted to the web site.

-Steve


>________________________________
>From: David Simcha <dsimcha at gmail.com>
>To: Discuss the phobos library for D <phobos at puremagic.com>
>Sent: Sunday, May 1, 2011 11:29 PM
>Subject: Re: [phobos] [D-Programming-Language/phobos] a15e68: Accidentally committed with a bunch of debugging c...
>
>On 5/1/2011 10:51 PM, Andrei Alexandrescu wrote:
>> David -- you may want to make updates through the pull request system. That gives you the opportunity of having one extra pair of eyes look over the code.
>> 
>> Ideally there should be at least two people involved in a change. Unfortunately, it looks like the second person is me rather often.
>> 
>
>I understand the idea that significant commits should be reviewed, but where do we draw the line?? Reviewing even the most trivial commits seems unnecessarily bureaucratic to me.? In the case of the initial commit, std.parallelism had already been in review for weeks.? (To play Devil's Advocate, though, the changes to the make file might have been worth reviewing.)
>_______________________________________________
>phobos mailing list
>phobos at puremagic.com
>http://lists.puremagic.com/mailman/listinfo/phobos
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20110502/f5af88c3/attachment.html>

> Guidelines for this would be nice to have.
> 
> I'd say definite changes that should go through a reviewed pull request:
> 
> * changes from non-phobos developers (obviously)
> * changes to files that you do not "own" (even if trivial).  For example,
> if I were to find a bug in std.parallelism, I shouldn't commit, I should
> generate a pull request for review.
> 
> * changes that alter the design of a module.
> * new code (including adding classes/structs/functions to an existing
> module)
> 
> 
> I think everything else is OK to commit directly instead of generating a pull request, including bug fixes (as long as they are not fixed by doing one of the above).
> 
> 
> What do you think?  A list like this (and actually, our entire process should be documented, including how to get included as a phobos developer) should be posted to the web site.

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.

My one real concern with pull requests though is that not enough people have been paying attention to them. On the whole, a fairly number of Phobos developers have been commenting on pull requests with any regularity, and it becomes more burdensome when there are very few people looking at pull requests. It also slows the process down more than it might otherwise.

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.

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.

- Jonathan M Davis

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20110502/7cb2e290/attachment.html>

> 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. The question is how we deal with and fix the problems that arise from them (particularly the delays), and whether _all_ code changes should go through pull requests. And as I said, I lean towards requiring that everything but the smallest of changes go through pull requests, but I agree that the current delays in that process are definitely problematic.

- Jonathan M Davis

> > 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. The question is how we deal with and fix the problems that arise from them (particularly the delays), and whether _all_ code changes should go through pull requests. And as I said, I lean towards requiring that everything but the smallest of changes go through pull requests, but I agree that the current delays in that process are definitely problematic.

I do think that that it's worth noting, however, that the sort of stuff that you've been working on lately, David, (i.e. std.parallelism) is likely much more dependent on the platform that it's running on than most of Phobos. Most of Phobos is quite platform independent, in which case, platform-specific bugs tend to be bugs in the compiler. So, that does tend to change your perspective and needs a bit. There are other modules (such as std.datetime) which are definitely affected by the platform that they're on, but most aren't. So, in the general case, the need to test code on multiple platforms shouldn't be anywhere near as critical as what you've been experiencing.

- Jonathan M Davis

>> 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.

Probably the matter of auto-testing should be best solved by improving the auto-tester.

One thing that has been learned at Facebook the hard way is that seemingly trivial changes can actually introduce pernicious bugs. Also, it's difficult to draw a line between what's trivial and what isn't. Today there is virtually no change to the main trunk that has not been seen by at least one extra person than the committer.

There aren't a lot of us but I think it shouldn't be difficult for each of us to look over and merge a pull request once in a while. Only in case a minor pull request just sits there for a while and repeated emails cause no answer, I think it's okay at our organization size to just allow the pull request author to pull the code.


Andrei

> >> 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.
> 
> Probably the matter of auto-testing should be best solved by improving the auto-tester.
> 
> One thing that has been learned at Facebook the hard way is that seemingly trivial changes can actually introduce pernicious bugs. Also, it's difficult to draw a line between what's trivial and what isn't. Today there is virtually no change to the main trunk that has not been seen by at least one extra person than the committer.
> 
> There aren't a lot of us but I think it shouldn't be difficult for each of us to look over and merge a pull request once in a while. Only in case a minor pull request just sits there for a while and repeated emails cause no answer, I think it's okay at our organization size to just allow the pull request author to pull the code.

One possible solution to the need to know whether code works on a particular platform would be to check in the code into a branch on github and then make a request on the newsgroup (be it the Phobos newsgroup or the main newsgroup) that someone with a particular platform pull the code and verify that it compiles and passes the unit tests. It's not instantaneous, but it could be that there's a good chance that someone would be able to test the code for you within a day or less (especially if you posted on the main newsgroup). That's not as nice as having a way to automatically verify that it works on all platforms, but it would be a way to verify that it works on other platforms without having to affect the master branch.

- Jonathan M Davis