Bug 62166 - Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations
Summary: Add a suggest-nominations command to webkit-patch for computing potential com...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tom Zakrajsek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-06 17:03 PDT by Eric Seidel (no email)
Modified: 2011-10-19 16:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.21 KB, patch)
2011-06-06 17:06 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
sorted the includes (10.14 KB, patch)
2011-06-06 17:08 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (15.13 KB, patch)
2011-09-15 14:23 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (23.72 KB, patch)
2011-10-12 16:28 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Patch (24.58 KB, patch)
2011-10-13 08:38 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2011-10-14 09:02 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-06-06 17:03:50 PDT
Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations
Comment 1 Eric Seidel (no email) 2011-06-06 17:06:46 PDT
Created attachment 96155 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-06 17:07:38 PDT
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.
Comment 3 Eric Seidel (no email) 2011-06-06 17:08:30 PDT
Created attachment 96156 [details]
sorted the includes
Comment 4 Eric Seidel (no email) 2011-06-06 17:10:33 PDT
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 5 Adam Barth 2011-06-06 17:12:07 PDT
Comment on attachment 96156 [details]
sorted the includes

I can haz tests?
Comment 6 Eric Seidel (no email) 2011-06-06 17:13:08 PDT
Sure, what would you like tested? :)
Comment 7 Adam Barth 2011-06-06 17:14:35 PDT
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.
Comment 8 Eric Seidel (no email) 2011-06-06 17:17:49 PDT
This has some relation to bug 26533, but I didn't re-use any of that code (because it was never landed into webkitpy).
Comment 9 Siddharth Mathur 2011-08-08 10:48:32 PDT
(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>
Comment 10 Eric Seidel (no email) 2011-09-15 14:23:22 PDT
Created attachment 107553 [details]
Patch
Comment 11 Adam Barth 2011-09-15 15:27:22 PDT
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?
Comment 12 Adam Barth 2011-09-15 15:27:52 PDT
Sorry, don't have time to review completely right now.
Comment 13 Eric Seidel (no email) 2011-09-15 15:29:03 PDT
I'm in no rush.  :)
Comment 14 Tom Zakrajsek 2011-09-16 15:40:33 PDT
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
                ^
Comment 15 Tom Zakrajsek 2011-09-16 16:15:19 PDT
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.
Comment 16 Eric Seidel (no email) 2011-09-16 16:18:45 PDT
We have this general problem with stdin/stdout encoding on systems with LANG=ascii set.  This is the same root problem as bug 63452.
Comment 17 Tom Zakrajsek 2011-09-21 10:25:55 PDT
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.
Comment 18 Tom Zakrajsek 2011-10-12 16:28:44 PDT
Created attachment 110769 [details]
Patch
Comment 19 Eric Seidel (no email) 2011-10-12 16:47:40 PDT
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?
Comment 20 Tom Zakrajsek 2011-10-12 21:07:12 PDT
(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.
Comment 21 Tom Zakrajsek 2011-10-13 08:38:00 PDT
Created attachment 110850 [details]
Patch
Comment 22 Tom Zakrajsek 2011-10-13 12:47:21 PDT
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 23 Eric Seidel (no email) 2011-10-13 13:07:11 PDT
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.
Comment 24 Tom Zakrajsek 2011-10-13 15:31:56 PDT
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.
Comment 25 Eric Seidel (no email) 2011-10-13 15:58:53 PDT
Split out the "setup this object from options" into a separate method and call it maually from both execute and your unittest.
Comment 26 Tom Zakrajsek 2011-10-14 09:02:01 PDT
Created attachment 111021 [details]
Patch
Comment 27 Eric Seidel (no email) 2011-10-19 14:03:08 PDT
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 28 WebKit Review Bot 2011-10-19 16:24:19 PDT
Comment on attachment 111021 [details]
Patch

Clearing flags on attachment: 111021

Committed r97891: <http://trac.webkit.org/changeset/97891>
Comment 29 WebKit Review Bot 2011-10-19 16:24:25 PDT
All reviewed patches have been landed.  Closing bug.