That's not an issue of "we need better contributions from new people", that's "We, the committers, need to be more diligent in reviewing and testing before committing".
We shouldn't be discouraging contributions from anyone, but we don't have to accept every pull request as-is, either. We can push back on the patch if it isn't clean enough, or if it isn't clear that it's the right solution, or even if it doesn't come with tests / docs. I'd have no problem with a lot more pull requests, if there was healthy discussion on JIRA for each one, including the core committers and the original reporter, before accepting (or rejecting) them. Regarding this particular commit, the original patch is ugly, and probably shouldn't have been accepted without some cleanup, review, and attached tests. The additional issue that it caused, though, is obviously something that wasn't covered by our existing tests. It was only noticed a couple of months later, in a post on StackOverflow, and as far as I can tell, it was only mentioned in Github 18 days ago, and Andrew was right on it. (He's not working on it currently, for other reasons) That regression doesn't seem to me to be a failure in process, so much as a gap in our test framework (which we can rectify easily enough) and maybe an issue of not-paying-enough-attention-to-stackoverflow, or not-having-a-clear-channel-to-report-bugs. Maybe if it were easier for people to tell us when something broke and we didn't catch it, we might have fixed this a month sooner. On Wed, Jun 12, 2013 at 2:09 AM, Joe Bowser <bows...@gmail.com> wrote: > Hey > > I decided to check my e-mail and respond to an issue. It turns out > that we're not properly testing or even checking the pull requests > that we're getting into the project. The bug in question is CB-3766, > which was caused by a patch accepted to fix CB-2458. The error isn't > totally obvious, and doesn't get easily picked up on unit test (our > CordovaWebView test mostly works, but fires a TIMING ERROR), but if > you use an emulator, due to the emulator's crappiness, it apparently > creates an ugly stack trace. > > Honestly, there's no way that this should have made it in. I > personally think that I'm going to have to rename loadUrlIntoView to > initAndLoadUrl or something more obvious, since it's too similar to > loadUrl. That being said, we need to be more strict about quality > while at the same time encouraging people to contribute. I'd rather > deal with hundreds of pull requests than hundreds of issues where > people know the answer but don't tell you. > > Any ideas how we can accomplish this? Has anyone else seen pull > requests get in that shouldn't have? > > Joe >
