cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Mocny <mmo...@chromium.org>
Subject Re: We need better contributions from new people
Date Wed, 12 Jun 2013 14:17:28 GMT
Before we go changing process, is this really a systemic issue?  Bugs
happen, and people sometimes land code, I'de rather be nimble in fixing
those issues than rigid in preventing them.  Personal opinion.

-Michal


On Wed, Jun 12, 2013 at 9:54 AM, Ian Clelland <iclelland@chromium.org>wrote:

> 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