[webkit-reviews] review denied: [Bug 103537] [webkitpy] Raise a ScriptError where it can be handled instead of directly using sys.exit : [Attachment 176521] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 103537: [webkitpy] Raise a ScriptError where it can be handled instead of
directly using sys.exit
https://bugs.webkit.org/show_bug.cgi?id=103537

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>

ScriptError is used by Executives to indicate that a called subprocess returned
an exit code; you're using this for totally different reasons that have nothing
to do with this, making the meaning of ScriptError much less clear. Many of the
changes in the commands seems like they could be replaced with a common
exception, but ScriptError is the wrong thing to use. Maybe add a generic
StepException or CommandException to indicate fatal errors in commands ...

Further, using the same exception for various command-related errors and
various internal errors seems confusing. For example, the change in
scm.ensure_no_local_commits() doesn't seem like it should be an exception at
all (since it seems like an error that could easily happen), and the change in
scm.commit_locally_with_message seems like it should be an AssertionError or a
NotImplementedError instead.


Lastly, it's not clear what a caller is supposed to when many of these errors
are raised; if ScriptError is raised; if there's nothing they can reasonably do
other than call sys.exit (which seems to be what happens in
abstractsequencedcommand.py), it doesn't seem like this is much of an
improvement over calling sys.exit directly.
ScriptError, on the other hand, is almost never used to indicate "I give up, so
you should exit"; most ScriptError exceptions are recoverable (frankly, I think
ScriptError is mostly used for bad reasons where the caller was being lazy :().