Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations
Created attachment 96155 [details] Patch
I wrote this while I was waiting for my fastMalloc(0) patch to finally run through all the layout tests this morning. Mostly because I had the surprising realization the other day that Noel Gordon was not yet a committer.
Created attachment 96156 [details] sorted the includes
Here is some example output (ignoring all the Warnings it currently prints due to missing email addresses in committers.py): % webkit-patch suggest-nominations REVIEWER: Patrick Gansterer (paroga@webkit.org) has 224 reviewed patches REVIEWER: Pavel Podivilov (podivilov@chromium.org) has 181 reviewed patches REVIEWER: Alexander Pavlov (apavlov@chromium.org) has 108 reviewed patches REVIEWER: Chris Rogers (crogers@google.com) has 107 reviewed patches REVIEWER: Carlos Garcia Campos (cgarcia@igalia.com) has 101 reviewed patches REVIEWER: Jer Noble (jer.noble@apple.com) has 100 reviewed patches REVIEWER: Ilya Tikhonovsky (loislo@chromium.org) has 88 reviewed patches REVIEWER: Philippe Normand (pnormand@igalia.com) has 86 reviewed patches REVIEWER: Abhishek Arya (inferno@chromium.org) has 82 reviewed patches COMMITTER: Andrey Adaikin (aandrey@google.com) has 40 reviewed patches COMMITTER: Ryuan Choi (ryuan.choi@samsung.com) has 36 reviewed patches COMMITTER: Vsevolod Vlasov (vsevik@chromium.org) has 36 reviewed patches COMMITTER: Luke Macpherson (macpherson@chromium.org) has 35 reviewed patches COMMITTER: Mark Pilgrim (pilgrim@chromium.org) has 32 reviewed patches COMMITTER: Ilya Sherman (isherman@chromium.org) has 25 reviewed patches COMMITTER: Siddharth Mathur (siddharth.mathur@nokia.com) has 24 reviewed patches COMMITTER: Mike Reed (reed@google.com) has 23 reviewed patches COMMITTER: Naoki Takano (takano.naoki@gmail.com) has 23 reviewed patches COMMITTER: Chris Guillory (chris.guillory@google.com) has 21 reviewed patches COMMITTER: Nat Duca (nduca@chromium.org) has 21 reviewed patches COMMITTER: Noel Gordon (noel.gordon@gmail.com) has 21 reviewed patches
Comment on attachment 96156 [details] sorted the includes I can haz tests?
Sure, what would you like tested? :)
Comment on attachment 96156 [details] sorted the includes I would just stub out git_log = self._tool.executive.run_command(['git', 'log', '--since="9 months ago"']) to produce interesting output that you can then see what your code prints.
This has some relation to bug 26533, but I didn't re-use any of that code (because it was never landed into webkitpy).
(In reply to comment #4) <joke>Gee, it's great to see a tool flag my name for *appreciation*. Wish life had more such tools :) </joke>
Created attachment 107553 [details] Patch
Comment on attachment 107553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107553&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:61 > + git_log = self._tool.executive.run_command(['git', 'log', '--since="9 months ago"']) > + match_git_svn_id = re.compile(r"\n\n git-svn-id:.*\n", re.MULTILINE) > + match_get_log_lines = re.compile(r"^\S.*\n", re.MULTILINE) > + match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE) This only works on git? Should we mention that in the help?
Sorry, don't have time to review completely right now.
I'm in no rush. :)
I'm not sure if it's a big deal, but this patch chokes if you redirect the output (at least on linux). Specifically when it is printing unicode names. $ webkit-patch suggest-nominations > temp.txt Traceback (most recent call last): File "/home/tomz/WebKit/Tools/Scripts/webkit-patch", line 66, in <module> main() File "/home/tomz/WebKit/Tools/Scripts/webkit-patch", line 61, in main WebKitPatch(os.path.abspath(__file__)).main() File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 308, in main result = command.check_arguments_and_execute(options, args, self) File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 117, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/commands/suggestnominations.py", line 145, in execute self._print_nominations(nominations) File "/home/tomz/WebKit/Tools/Scripts/webkitpy/tool/commands/suggestnominations.py", line 140, in _print_nominations print "%(roles_string)s: %(author_name)s (%(author_email)s) has %(patch_count)s reviewed patches" % nomination UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 16: ordinal not in range(128) When I don't redirect, I can see that the name it choked on was: COMMITTER: Kristóf Kosztyó (Kosztyo.Kristof@stud.u-szeged.hu) has 19 reviewed patches ^
You could append ".encode('ascii', 'replace')" to author_name at suggestnominations.py@121. 121: 'author_name': author_name.encode('ascii', 'replace'), The downside is that those characters become '?' even when not using redirection. Still seems like a better idea than a potential exception.
We have this general problem with stdin/stdout encoding on systems with LANG=ascii set. This is the same root problem as bug 63452.
There are some people that have contributed using different emails. Right now, this script doesn't take the union of those. One fix would be to count patches by Name.
Created attachment 110769 [details] Patch
Comment on attachment 110769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110769&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:58 > + options = [ > + steps.Options.committer_minimum, > + steps.Options.reviewer_minimum, > + steps.Options.max_commit_age, > + steps.Options.show_commits, > + ] > + > + AbstractDeclarativeCommand.__init__(self, options=options) I'm confused why we need to do this dance?
(In reply to comment #19) > (From update of attachment 110769 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110769&action=review > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:58 > > + options = [ > > + steps.Options.committer_minimum, > > + steps.Options.reviewer_minimum, > > + steps.Options.max_commit_age, > > + steps.Options.show_commits, > > + ] > > + > > + AbstractDeclarativeCommand.__init__(self, options=options) > > I'm confused why we need to do this dance? Assuming you mean the __init__() call, this was in your first draft, and without it bind_to_tool fails with "AttributeError: 'SuggestNominations' object has no attribute '_tool'". The original version was supporting an option "git_commit" which I didn't see a reason for though.
Created attachment 110850 [details] Patch
Adam Barth thought the issue in #19 was about the options setup, and recommended pointing to another example of this pattern. See Tools/Scripts/webkitpy/tool/commands/upload.py: PostCommits, MarkBugFixed, CreateBug, ....
Comment on attachment 110850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110850&action=review One more round please. :) > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48 > + self.committer_minimum = steps.Options.committer_minimum.default > + self.reviewer_minimum = steps.Options.reviewer_minimum.default > + self.max_commit_age = steps.Options.max_commit_age.default > + self.show_commits = steps.Options.show_commits.default Why do you need to pull defaults here? > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:55 > + steps.Options.committer_minimum, > + steps.Options.reviewer_minimum, > + steps.Options.max_commit_age, > + steps.Options.show_commits, Yeah, since you're not using steps, you could just do the make_option here, isntead of using the shared steps options. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:60 > + # FIXME: This should probably be on the tool somewhere. > + self._committer_list = CommitterList() Isn't it already on the tool? Maybe it isn't... but it certainly should be. :) (Not that you have to fix that.) > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:162 > + for author_email, curr_counter in counters_by_email.items(): curr_ is a horrible name. just call it "counter" or "counter_for_author" or whatever it is. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:163 > + if author_email != curr_counter['mru_email']: what's mru? most recently used? maybe latest_email? latest_name? > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:199 > + curr_counter = self._counters_by_email[nomination['author_email']] yeah, just call it counter. :) > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209 > + # split the tuples > + # the second element is the "counter" structure > + ia, a = a_tuple > + ib, b = b_tuple You want somethign like: _, a_counter = a_tuple _, b_counter = b_tuple > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:225 > + aka_list = [] I might have called this alais_list. aka is OK, but might be more confusing to non-native english speakers. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:230 > + if len(aka_list): You can just do if aka_list here. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:248 > + two lines. > Tools/Scripts/webkitpy/tool/steps/options.py:41 > + committer_minimum = make_option("--committer-minimum", action="store", dest="committer_minimum", type="int", default=10, help="Specify minimum patch count for Committer nominations.") I wouldn't put these in steps/options unless they're used by a step. Just inline them into your command.
Awesome feedback. Thanks. I'm working on fixing. I'm having trouble annotating your review comments though. Specifically: Why do you need to pull defaults here? > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:55 > + steps.Options.committer_minimum, it's because one of the unit tests is bypassing the "exec" function. It just creates the object and calls _recent_commit_messages to test. I could restructure that test. I just thought that initializing these here made sense in the spirit of don't create an object in an invalid state. They are "overridden" if exec is passed new values. I'll see if I can come up with a better way.
Split out the "setup this object from options" into a separate method and call it maually from both execute and your unittest.
Created attachment 111021 [details] Patch
Comment on attachment 111021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111021&action=review I'm willing to rubber-stamp this, so we can use it and evaluate the utility. Seems like a huge amount of code for this. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:78 > + (message, _) = match_git_svn_id.subn("", message) > + (message, _) = match_get_log_lines.subn("", message) > + (message, _) = match_leading_indent.subn("", message) Why subn and not sub? Seems you don't want the number of subs anyway. :)
Comment on attachment 111021 [details] Patch Clearing flags on attachment: 111021 Committed r97891: <http://trac.webkit.org/changeset/97891>
All reviewed patches have been landed. Closing bug.