Pull requests report (17/7/2013) View as Markdown

Hi all,

2013/7/18 Yasuo Ohgaki yohgaki@ohgaki.net

2013/7/18 Sherif Ramadan theanomaly.is@gmail.com

First off, thanks for all the hard work. PRs aren't getting as much
attention as they should, and I'd like to say that I certainly haven't
been
helping as much as I should with them.

We may have bugs ML and PR ML so that any discussion, then it may
have more attentions.

Bugs ML may have too many mails, but PR ML would have much less mails.
It may be good idea that redirect PR discussion to internals, if it is
possible.

We already have Bugs ML

http://www.php.net/mailing-lists.php

I subscribed it now.

For github, there is notification, but apparently not many people pay
attention.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Hi Sherif,

2013/7/18 Sherif Ramadan theanomaly.is@gmail.com

First off, thanks for all the hard work. PRs aren't getting as much
attention as they should, and I'd like to say that I certainly haven't
been
helping as much as I should with them.

We may have bugs ML and PR ML so that any discussion, then it may
have more attentions.

Bugs ML may have too many mails, but PR ML would have much less mails.
It may be good idea that redirect PR discussion to internals, if it is
possible.

Well, some people may not be paying attention to the discussion on github
about certain PRs and that's OK. Some people may not fully understand or
have enough knowledge in one particular area to be able to help with that
PR. Perfectly fine.

However, I don't think that THIS is the reason PRs aren't getting their
fair share of attention, honestly. For example, look at the oldest PR
sitting around for over a year now
https://github.com/php/php-src/pull/15 plenty
of discussion there. Lots of criticism, suggestions, etc... All of that is
wonderful. People are participating!

Then all of a sudden the people involved in the discussion seem to
mysteriously disappear and nothing happens to the PR. It just sits in limbo
indefinitely. This is not the only PR I've seen follow this pattern.

I assure you the discussion on github and the mailing lists are not the
reason we've got stagnate PRs.

On Thu, Jul 18, 2013 at 4:21 AM, Sherif Ramadan theanomaly.is@gmail.comwrote:

What do you think about closing older PR ( > 28 days) ?

First off, thanks for all the hard work. PRs aren't getting as much
attention as they should, and I'd like to say that I certainly haven't been
helping as much as I should with them. What I am noticing though, is that a
lot of these newer PRs, or the ones that are getting more attention, seem
to be fairly cosmetic changes. I mean, spelling mistakes are all great
fixes, and no disrespect to any contribution small or big, but there are
certainly a lot of PRs which people have spent a great deal of time working
on, clearly, that seem to be getting ignored. If we start closing these
strictly because of how long they've been open it would be such a
discouragement to everyone who contributed. I sincerely hope this doesn't
happen. I'd much rather see people spending more time on reviewing some of
the older PRs and seeing if they're worthy of merging or not based on
substance rather than just dismiss them based on how long they've been open.

I've tried to get in contact with some of the original authors of at
least a couple of PRs in the past weeks to see if they were interested in
working on an RFC to get their PRs merged as they necessitated some more
discussion, but so far have come up empty. So I'll try to help out as much
as I can with the ones that can get immediate attention.

I'm glad to see that question got everyone's attension (:

Indeed we started with the easy ones... there isn't any reason simple
patches
won't get merged very quickly.

No disagreement about doing what can be done.

We have some PR which the disscussion about them is stuck, the question is
who
and when do we cut it off and send the author to rework his patch. After
some
feedback was given, if the author isn't responsive, and no one else wants
to
handle the patch instead, we can reject and ask them to come back when
ready.

As you can see from my example (of which are are numerous others), the
author did everything that was asked of them. Yet, surprisingly, some
people only seem to have something to add to the discussion when there is a
problem (no harm there in pointing out problems with the patch), but are
nowhere to be seen when there is nothing left to be critical of (definite
bad on our part for not following up after the author did their part).

Also, we have more than a few PR which the patch is OK, but are stuck due
to a
missing test. This is fair enough, but should also have some rule of thumb
for
these cases.

True, a lot of people either don't know how to write tests or don't like to
write tests for our test suite. They also might not know how to fix the
test in some cases where changing existing tests in necessary. This isn't
just limited to contributors not members of the PHP project, but even those
that have been long time contributors have come to me with a request to
either fix or write the test for them. I'll admit it took me a bit to get
the hang of phpt's work flow.

Seeing a long list of PR waiting for a year isn't much encouraging, I
would
prefer to see people quickly either have their changes accepted, sent to
improve
the PR or completly rejected. Don't forget the PR can always be resent and
the
work isn't lost.

Seeing PRs closed because no one cares to look at them is not ideal either.
I agree that we should respond as quickly as possible, but sometimes
changes being submitted require a bit more discussion on internals and
people seem reluctant to take this step. I can't blame them, internals can
be quite intimidating.

I, myself, have experience the pain-staking work it takes to get my PRs
merged into php-src, in the past. It took me nearly 6 months for some, and
some sat stagnate for longer until I decided to close them myself since the
people requesting the feedback never seemed to get back to me after
feedback was provided.

The process of getting PRs merged is definitely an on-going challenge due
to limited man-power and factors of impeding communication. No doubt about
that. I do not fault anyone in particular. I'd just like to see that
everyone gets a fair shot and no one is excluded for the sake of exclusion
or quickly drilling down the open PR list.

Hi!

Indeed we started with the easy ones... there isn't any reason simple
patches
won't get merged very quickly.

There's a very simple reason - there are not enough people with enough
knowledge about PHP internals to properly evaluate patches that do it on
regular basis. Even the simplest patch takes time, at least if you do it
the right way - you have to read it, understand if it's right, pull it,
build with it, run tests with it if it has tests, merge it to multiple
branches, etc. - this takes time. Time is a scarce resource.

We have some PR which the disscussion about them is stuck, the question is
who
and when do we cut it off and send the author to rework his patch. After

Why we need to "cut off" anything? I mean, if the case is obvious - like
the author does not answer the feedback for many months - we could close
it - but if the patch is being worked on, why should we remove it from
public view? Some things go fast, some things take time. I think we need
to go case by case.

Also, we have more than a few PR which the patch is OK, but are stuck due
to a
missing test. This is fair enough, but should also have some rule of thumb
for
these cases.

Again, it would not be hard to add tests for many of them, the problem
is again - time. For myself, I'd rather spend time on merging properly
done patches or fixing bugs than finishing up patches that the original
author could finish up. But if some people decide that this is a good
way for them to contribute - that'd be great. Otherwise, removing these
pulls from public view would only mean they'd never get taken care at all.

Seeing a long list of PR waiting for a year isn't much encouraging, I would
prefer to see people quickly either have their changes accepted, sent to
improve
the PR or completly rejected. Don't forget the PR can always be resent and
the
work isn't lost.

The work isn't lost but once PR is closed the chance anybody would look
at it again except original author is very low. While if it's open, it
still could be a chance somebody would pick it up.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227