[webkit-reviews] review denied: [Bug 115387] [webkitpy] Need suggest-emeriti command : [Attachment 200516] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Glenn Adams
<glenn at skynav.com>'s request for review:
Bug 115387: [webkitpy] Need suggest-emeriti command
https://bugs.webkit.org/show_bug.cgi?id=115387

Attachment 200516: Patch
https://bugs.webkit.org/attachment.cgi?id=200516&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.