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
Created attachment 200509 [details] Patch
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.
Created attachment 200516 [details] Patch
(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.
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.
Would be nice to revive this.