[webkit-reviews] review denied: [Bug 51176] sheriff-bot should be able to do multi-revision rollouts : [Attachment 76757] proposed patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Gabor Rapcsanyi
<rgabor at inf.u-szeged.hu>'s request for review:
Bug 51176: sheriff-bot should be able to do multi-revision rollouts
https://bugs.webkit.org/show_bug.cgi?id=51176

Attachment 76757: proposed patch
https://bugs.webkit.org/attachment.cgi?id=76757&action=review

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

Needs another round. :)

> WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py:64
> +	   read_revision = True
> +	   rollout_reason = []
> +	   # the first argument must be a revision number
> +	   svn_revision_list = [args[0].lstrip("r")]
> +	   if not svn_revision_list[0].isdigit():
> +	       read_revision = False
> +

Please split this (arg parsing, etc.) into private (_foo()) helper functions
instead of just making execute longer.

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:58
> +	       svn_revisions = " ".join([str(int(revision)) for revision in
svn_revision_list])

Um, why do str(int(?

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:76
> +	       svn_revisions,

Please pass them individually as numbers converted to strings, instead of as a
joined string.