Output a list of suggested OWNERS reviewers when needed.

Output a list of suggested OWNERS reviewers when needed.

BUG=None
TEST=Change files that need OWNERS review.  Upload the patch.  Check that the warning suggests a minimum set of reviewers.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135619

[Zachary Kuznia, 2012-04-26 07:12:23]

Hello nsylvain,

Could you review this at:
https://chromiumcodereview.appspot.com/10222020

Thanks,
-Zach

[M-A Ruel, 2012-04-26 15:48:48]

git blame tells me Dirk is the right reviewer for owners.py.

[Dirk Pranke, 2012-04-27 00:00:13]

Thanks for working on this!

After starting at the algorithm for a while, I think it's probably correct
although I have no idea how comparatively efficient it is (or if it matters).
Have you run this over any representative CLs to see what happens if we span
more than a couple directories?

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py
File owners.py (right):

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode182
owners.py:182: current_owners.add(owner)
nit: this can just be current_owners |= self.owners_for[dir_name]

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode194
owners.py:194: owned_dirs[owner].add(current_dir)
nit: lines 192-194 would be a little more idiomatic as:

  owned_dirs.get(owner, set()).add(current_dir)

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode207
owners.py:207: max_owner = owner
I think if you have cases where the same set of directories is covered by the
same set of owners you will always pick the same person and the other owners
will get "starved". I wonder if we want to randomize this choice ...

https://chromiumcodereview.appspot.com/10222020/diff/4001/tests/owners_unitte...
File tests/owners_unittest.py (left):

https://chromiumcodereview.appspot.com/10222020/diff/4001/tests/owners_unitte...
tests/owners_unittest.py:143: 
It should be easy enough to add a couple more tests here for changes with that
span multiple files or multiple folders ... can you please add them? There's not
really a good reason we shouldn't have decent test coverage here, and I prefer
that to the manual testing you described in the TEST= line.

[Zachary Kuznia, 2012-05-02 08:57:58]

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py
File owners.py (right):

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode182
owners.py:182: current_owners.add(owner)
On 2012/04/27 00:00:13, Dirk Pranke wrote:
> nit: this can just be current_owners |= self.owners_for[dir_name]

Done.

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode194
owners.py:194: owned_dirs[owner].add(current_dir)
On 2012/04/27 00:00:13, Dirk Pranke wrote:
> nit: lines 192-194 would be a little more idiomatic as:
> 
>   owned_dirs.get(owner, set()).add(current_dir)
> 

Done.

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode207
owners.py:207: max_owner = owner
Next week, I'm going to look at adding prioritization for the deepest nested
owners.  Do you think that would be sufficient?

On 2012/04/27 00:00:13, Dirk Pranke wrote:
> I think if you have cases where the same set of directories is covered by the
> same set of owners you will always pick the same person and the other owners
> will get "starved". I wonder if we want to randomize this choice ...

https://chromiumcodereview.appspot.com/10222020/diff/4001/tests/owners_unitte...
File tests/owners_unittest.py (left):

https://chromiumcodereview.appspot.com/10222020/diff/4001/tests/owners_unitte...
tests/owners_unittest.py:143: 
On 2012/04/27 00:00:13, Dirk Pranke wrote:
> It should be easy enough to add a couple more tests here for changes with that
> span multiple files or multiple folders ... can you please add them? There's
not
> really a good reason we shouldn't have decent test coverage here, and I prefer
> that to the manual testing you described in the TEST= line.

Done.

[Dirk Pranke, 2012-05-02 21:43:31]

lgtm.

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py
File owners.py (right):

https://chromiumcodereview.appspot.com/10222020/diff/4001/owners.py#newcode207
owners.py:207: max_owner = owner
On 2012/05/02 08:57:58, Zachary Kuznia wrote:
> Next week, I'm going to look at adding prioritization for the deepest nested
> owners.  Do you think that would be sufficient?
> 

Dunno. Possibly. I suggest launch early and iterate often :) We'll see who
complains.