[webkit-reviews] review denied: [Bug 62166] Add a suggest-nominations command to webkit-patch for computing potential committer/reviewer nominations : [Attachment 110850] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Tom Zakrajsek <tomz at codeaurora.org>'s
request for review:
Bug 62166: Add a suggest-nominations command to webkit-patch for computing
potential committer/reviewer nominations
https://bugs.webkit.org/show_bug.cgi?id=62166

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.