cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Woghiren <m...@chromium.org>
Subject Re: We need better contributions from new people
Date Wed, 12 Jun 2013 14:35:43 GMT
I agree with Ian—reviewing and testing just needs to be more thorough.


On Wed, Jun 12, 2013 at 10:17 AM, Michal Mocny <mmocny@chromium.org> wrote:

> 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