[webkit-reviews] review denied: [Bug 47466] new-run-webkit-tests: handle missing ruby/prettypatch better : [Attachment 70389] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 47466: new-run-webkit-tests: handle missing ruby/prettypatch better
https://bugs.webkit.org/show_bug.cgi?id=47466

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70389&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:131
> +    def check_pretty_patch(self, override_step=None, logging=True):

What are the parameters for? override_step doesn't seem to be used and logging
is not set by the single caller of this function.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:673
> +	   command = ["ruby", "-I", os.path.dirname(self._pretty_patch_path),
> +		      self._pretty_patch_path, diff_path]

Nit: Use a tuple instead of a list since we don't plan on mutating this.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> +	   if not self._pretty_patch_available:
> +	       _log.error('')

Why an empty error?  It looks like check_pretty_patch already logs a bunch of
stuff, so do we really need this?