On Thu, Jul 18, 2013 at 3:54 AM, Lior Kaplan <lio...@zend.com> wrote:
> On Thu, Jul 18, 2013 at 4:21 AM, Sherif Ramadan > <theanomaly...@gmail.com>wrote: > >> On Wed, Jul 17, 2013 at 6:47 PM, Lior Kaplan <lio...@zend.com> wrote: >> >>> 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.
