On Thu, 23 Aug 2012 05:19:39 -0700, Platonides Platonides@gmail.com wrote:
On 23/08/12 05:22, Jeremy Baron wrote:
Hi,
On Thu, Aug 23, 2012 at 2:46 AM, MZMcBride wrote:
Perhaps revisions that receive no feedback for thirty days should be auto-merged and auto-deployed.
That's a pretty horrible solution. IMO, all code *must* be reviewed[0] before merge.
I actually like it. If "Evil approval bot" mailed you warning that it will merge 12 pending changesets in two days if there's no action from your part, that would force some promptly action by you.
I recently had a trivial patch to operations/puppet waiting for more than a month. When I noticed I hadn't added any Reviewer, and added to it, the changeset was fixed in the same day. But that also shows that nobody looked for new changes there.
I have also seen people approving their own commits to core, something I'm not comfortable with.
We could add some rules like: "If someone with approval to core has only positive votes, he can approve it after 1 week. If there was no review at all, he can approve it after 2 weeks".
(Push repositories would obviously still allow auto-approval, and we could add exceptions for eg. translation changes)
I was also recently unhappy when I discovered that one patch I thought I had open, had been abandoned without explanation. There can be good reasons for doing that, this is a bad idea, no longer needed, fixed in a different way in I123456... or even "closing because it has been waiting for a new patch for too long and I don't like seeing this open" (which I suspect was the case), but *please*, if you're closing another people's patch, leave an explanation!
I don't know if I quite like the thought of an evil reviewer bot.
Though it does make me think of a feature I thought of adding to Gareth.
I can't remember why but one day this feature idea randomly popped into mind. Review tagging. Basically things for review can be tagged with a number of tags. For us we may create tags like (parser, api, skin, specialpages, i18n, etc...) and anyone can add these tags to the review item. Now besides making things searchable users can star/watch tags rather than only star/watch individual review items. This means that Gareth could automatically notify you about new commits in a part of MW that you are stalking/obsessed with. ((This is something we have always basically been missing)) By getting reviewers to watch their own area of MW they can automatically jump in to review changes in that area. Without needing to hover over a dashboard of changes or be overwhelmed by notifications of every single commit coming in. And getting your commit reviewed wouldn't be anymore a matter of "Try to find one of the few people in our large community who reviews the topic your commit is in" but instead would be the much simpler "Tag your commit with the relevant topics so that reviewers can see it".
Now, naturally as in the spirit of Gareth I didn't stop there with manual tagging and thought of a rules system to add tags automatically: if filechanged includes/parser/* add tag 'parser' if filechanged includes/api/* add tag 'api' if filechanged (includes/Skin.php, includes/SkinTemplate.php, includes/SkinLegacy.php) add tag 'skin' etc...
We could basically have tags automatically added based on what files were updated in the commit.
Personally I'd probably be hovering over any commit tagged with skin, linker, and a few other tags.