On Wed, Feb 19, 2014 at 12:38 PM, Felix S. Klock II <[email protected]>wrote:
> > On 19/02/2014 21:12, Flaper87 wrote: > > 2. Approval Process > > [...] For example, requiring 2 r+ from 2 different reviewers instead of > 1. This might seem a bit drastic now, however as the number of contributors > grows, this will help with making sure that patches are reviewed at least > by 2 core reviewers and they get enough attention. > > > I mentioned this on the #rust-internals irc channel but I figured I should > broadcast it here as well: > > regarding fractional r+, someone I was talking to recently described their > employer's process, where the first reviewer (who I think is perhaps part > of a priveleged subgroup) assigned the patch with the number of reviewers > it needs so that it isn't a flat "every patch needs two reviewers" but > instead, someone says "this looks like something big/hairy enough that it > needs K reviewers" > >From my personal experience with big companies, this is how it works. Having a policy where you require more than one reviewer for everything is too much process and wasted time; it's the big changes that require more than one person reviewing that get this level of attention, usually with the first reviewer going "hey X, could you take a look at this as well?". And having a every pull request auto-tested as soon as it is sent out would be wonderful; Travis CI is great for this (it isolates and times-out builds if needed), but obviously couldn't work for Rust. A "b+" meaning "please build this" seems like the second best thing. > > just something to consider, if we're going to look into strengthening our > review process. > > Cheers, > -Felix > > > On 19/02/2014 21:12, Flaper87 wrote: > > Hi all, > > I'd like to share some thoughts with regard to our current test and > approval process. Let me break this thoughts into 2 separate sections: > > 1. Testing: > > Currently, all patches are being tested after they are approved. However, > I think it would be of great benefit for contributors - and reviewers - to > test patches before and after they're approved. Testing the patches before > approval will allow folks proposing patches - although they're expected to > test the patches before submitting them - and reviewers to know that the > patch is indeed mergeable. Furthermore, it will help spotting corner cases, > regressions that would benefit from a good discussion while the PR is hot. > > I think we don't need to run all jobs, perhaps just Windows, OSx and > Linux should be enough for a first test phase. It would also be nice to run > lint checks, stability checks etc. IIRC, GH's API should allow us to notify > this checks failures. > > 2. Approval Process > > I'm very happy about how patches are reviewed. The time a patch waits > before receiving the first comment is almost 0 seconds and we are spread in > many patches. If we think someone else should take a look at some patch, we > always make sure to mention that person. > > I think the language would benefit from a more strict approval process. > For example, requiring 2 r+ from 2 different reviewers instead of 1. This > might seem a bit drastic now, however as the number of contributors grows, > this will help with making sure that patches are reviewed at least by 2 > core reviewers and they get enough attention. > > > I think both of these points are very important now that we're moving > towards 1.0 and the community keeps growing. > > Thoughts? Feedback? > > -- > Flavio (@flaper87) Percoco > http://www.flaper87.com > http://github.com/FlaPer87 > > > _______________________________________________ > Rust-dev mailing > [email protected]https://mail.mozilla.org/listinfo/rust-dev > > > > -- > irc: pnkfelix on irc.mozilla.org > email: {fklock, pnkfelix}@mozilla.com > > > _______________________________________________ > Rust-dev mailing list > [email protected] > https://mail.mozilla.org/listinfo/rust-dev > >
_______________________________________________ Rust-dev mailing list [email protected] https://mail.mozilla.org/listinfo/rust-dev
