Bug 107528 - webkit-patch suggest-reviewers should limit itself to 5 reviewers
Summary: webkit-patch suggest-reviewers should limit itself to 5 reviewers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 107525
  Show dependency treegraph
 
Reported: 2013-01-22 02:00 PST by Eric Seidel (no email)
Modified: 2013-02-15 12:47 PST (History)
4 users (show)

See Also:


Attachments
Preliminary patch (5.73 KB, patch)
2013-01-23 07:42 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2013-01-24 02:15 PST, Zan Dobersek
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2013-01-22 02:00:00 PST
webkit-patch suggest-reviewers should limit itself to 5 reviewers

Right now it will CC an unbounded number, which is bad.

Those should also be the 5 most relevant reviewers, ideally.  Easy to calculate based on number of annotated lines in the diff attributed to each reviewer.
Comment 1 Zan Dobersek 2013-01-23 06:37:55 PST
(In reply to comment #0)
> Those should also be the 5 most relevant reviewers, ideally.  Easy to calculate based on number of annotated lines in the diff attributed to each reviewer.

How about deciding about relevant reviewers by picking five who most recently reviewed patches around the code? This kind of guarantees that the offered reviewers is up-to-date with the current status of the code that's changed.

I think that's also easier to implement than sorting by the sum of reviewed lines.
Comment 2 Zan Dobersek 2013-01-23 07:42:37 PST
Created attachment 184231 [details]
Preliminary patch
Comment 3 Zan Dobersek 2013-01-23 07:46:55 PST
(In reply to comment #2)
> Created an attachment (id=184231) [details]
> Preliminary patch

A WIP patch that outlines the 'five-latest-reviewers' approach. No unit test breakage. I believe this keeps the current behavior though it's probably not in line with what bug #107525 would like to accomplish.
Comment 4 Eric Seidel (no email) 2013-01-23 10:59:13 PST
Comment on attachment 184231 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184231&action=review

> Tools/Scripts/webkitpy/common/checkout/checkout.py:146
> +        reviewers = []
> +        for info in commit_infos:
> +            if info.reviewer() and info.reviewer() not in reviewers:
> +                reviewers.append(info.reviewer())
> +            if info.author() and info.author().can_review and info.author() not in reviewers:
> +                reviewers.append(info.author())
> +        return reviewers

It seems all you did here was remove the list comprehensions?
Comment 5 Zan Dobersek 2013-01-23 12:01:10 PST
Comment on attachment 184231 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184231&action=review

>> Tools/Scripts/webkitpy/common/checkout/checkout.py:146
>> +        return reviewers
> 
> It seems all you did here was remove the list comprehensions?

This ugly piece of code actually just tries to use a list as an ordered set. The returned list contains all the reviewers connected to the changed files but lists them in order of their last activity, from most to least recent.
Comment 6 Zan Dobersek 2013-01-24 02:15:15 PST
Created attachment 184447 [details]
Patch
Comment 7 Eric Seidel (no email) 2013-02-15 12:38:37 PST
Comment on attachment 184447 [details]
Patch

OK.
Comment 8 Zan Dobersek 2013-02-15 12:47:33 PST
Committed r143033: <http://trac.webkit.org/changeset/143033>