cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ian Clelland <iclell...@chromium.org>
Subject Re: We need better contributions from new people
Date Wed, 12 Jun 2013 13:54:12 GMT
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 <bowserj@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message