[webkit-reviews] review granted: [Bug 106629] Extend sheriffbot's "help" command to be able to get help on individual commands : [Attachment 182714] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Alan Cutter
<alancutter at chromium.org>'s request for review:
Bug 106629: Extend sheriffbot's "help" command to be able to get help on
individual commands
https://bugs.webkit.org/show_bug.cgi?id=106629

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182714&action=review


This is fantastic!  So much better than what we currently have.  We could
quibble about the exact help strings, but this is also OK to land as is.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:58
> +    @classmethod

I think these are OK as classmethods (definitely more easily mocked than static
methods!), but still better as instance methods unless you need them to be
class methods for some usage.  I used to be big on static/class methods in
python, until I realized they were a huge pain to test/mock well.  BUt maybe
I'm just poor at testing. :)

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:124
> +    help_string = "Restarts sheriffbot."

I might even give a little more info. "Restarts sherrifbot.  Will update its
WebKit checkout, and re-join the channel momentarily."

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:171
> +    help_string = "Creates a patch for the reverse diff of the given
revision(s) and flags it as commit-queue?."

"Opens a rollout bug, CCing author + reviewer, and attaching the reverse-diff
of the given revisions marked as commit-queue=?" might give a little more info?


> Tools/Scripts/webkitpy/tool/bot/irc_command.py:292
> +    help_string = "Searches the known contributors/committers/reviewers for
additional information about them."

"Searches known contributors and returns any matches with irc, email and full
name."?  I'm not sure that's better, but "contributors" implies the other two
categories. :)