Bug 115387 – [webkitpy] Need suggest-emeriti command

• Component: Tools / Tests
• Status: NEW

[Glenn Adams]

There should be a suggest-emeriti command that complements to suggest-nominations, and which suggests committers and/or reviewers to be "retired" to an emeritus role.

See the thread at [1].

[1] https://lists.webkit.org/pipermail/webkit-dev/2013-April/024450.html

[Glenn Adams]

Created attachment 200509 [details]
Patch

[Benjamin Poulain]

Comment on attachment 200509 [details]
Patch

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

Really cool!

I think you could have get away with a single command :)

I cannot review this right now but here is some comments I have from looking over the patch:

> Tools/ChangeLog:59
> +        (SuggestNominations._count_recent_patches): Add require_reviews option, which differs from suggest-nominations and suggest-emeriti.

Why not just catch the exception from the call site in SuggestEmeriti?

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:2
> +# Copyright (c) 2011 Google Inc. All rights reserved.
> +# Copyright (c) 2011 Code Aurora Forum. All rights reserved.

It this right?

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:115
> +            # skip committers that are reviewers

Comment style.

[Glenn Adams]

Created attachment 200516 [details]
Patch

[Glenn Adams]

(In reply to comment #2)
> (From update of attachment 200509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200509&action=review
> 
> Really cool!
> 
> I think you could have get away with a single command :)

I suppose you mean I could have implemented this as an option on suggest-nominations. Yes, that would have been possible, at the cost of merging somewhat unrelated functionality. I prefer smaller modules with less surface area, making mods easier, etc.

> 
> I cannot review this right now but here is some comments I have from looking over the patch:
> 
> > Tools/ChangeLog:59
> > +        (SuggestNominations._count_recent_patches): Add require_reviews option, which differs from suggest-nominations and suggest-emeriti.
> 
> Why not just catch the exception from the call site in SuggestEmeriti?

In SuggestNominations, we need to treat missing reviewer as an error condition, raising exception without returning a commit dict; but in SuggestEmeriti lack of reviewer isn't an error, and we want the commit object. I suppose I could handle this by returning a commit object in both cases, and have SuggestNominations merely skip it if reviewers is empty. In which case I could remove the CommitLogMissingReviewer class.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:2
> > +# Copyright (c) 2011 Google Inc. All rights reserved.
> > +# Copyright (c) 2011 Code Aurora Forum. All rights reserved.
> 
> It this right?

Fixed. I thought of this after I posted the earlier patch.

> 
> > Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:115
> > +            # skip committers that are reviewers
> 
> Comment style.

Fixed.

[Benjamin Poulain]

Comment on attachment 200516 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:72
> +            reviewers_cache = activity['reviewers_cache']

Do we really need this? It is not like performance matters here.

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:80
> +                reviewer_name, _ = self._comment_regexp.subn("", reviewer_name)
> +                reviewer_name, _ = self._string_regexp.subn("", reviewer_name)
> +                reviewer_name = reviewer_name.strip()
> +                reviewer = reviewers_cache.get(reviewer_name)
> +                if not reviewer:
> +                    contributors, _ = self._committer_list.contributors_by_fuzzy_match(reviewer_name)
> +                    if contributors:

Discussed on IRC: looks like we should use the generalized cleaning code here.

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:88
> +                if not reviewed_revisions:
> +                    reviewed_revisions = set()

You can use defaultdict or similar mechanism to avoid this kind of tricks.

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:94
> +            committer = commit['committer']
> +            if committer:

Can "not committer" happen?

> Tools/Scripts/webkitpy/tool/commands/suggestemeriti.py:98
> +                if not committed_revisions:
> +                    committed_revisions = set()

Ditto.

[Simon Fraser (smfr)]

Would be nice to revive this.