Bug 180820 - [webkitpy] Add ignore_errors keyword argument to Executive.run_command() for replacing the common pattern of error_handler usage
Summary: [webkitpy] Add ignore_errors keyword argument to Executive.run_command() for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-14 11:15 PST by Basuke Suzuki
Modified: 2018-01-23 11:32 PST (History)
9 users (show)

See Also:


Attachments
patch (22.06 KB, patch)
2017-12-14 11:48 PST, Basuke Suzuki
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-12-14 11:15:25 PST
The error_handler argument of Executive.run_command() method is commonly used for specifying Executive.ignore_error to ignore errors. Is should be more clear by adding ignore_errors=True keyword argument.
Comment 1 Basuke Suzuki 2017-12-14 11:16:46 PST
Also there's no need to import Executive module just to pass Executive.ignore_error static method if this option is available.
Comment 2 Basuke Suzuki 2017-12-14 11:48:50 PST
Created attachment 329373 [details]
patch
Comment 3 David Kilzer (:ddkilzer) 2018-01-23 10:42:39 PST
Comment on attachment 329373 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329373&action=review

r=me with a couple comments.

> Tools/Scripts/webkitpy/common/system/executive.py:407
> +            if ignore_errors:
> +                assert error_handler is None, "don't specify error_handler if ignore_errors"

Nit:  I would add " is True" to the end of the message to make it clear:

            assert error_handler is None, "don't specify error_handler if ignore_errors is True"

I would prefer this to be fixed before landing unless you feel strongly about not adding " is True".

> Tools/Scripts/webkitpy/common/system/executive_mock.py:198
> +        if ignore_errors:
> +            assert error_handler is None, "don't specify error_handler if ignore_errors"

Nit:  I would add " is True" to the end of the message to make it clear:

            assert error_handler is None, "don't specify error_handler if ignore_errors is True"

I would prefer this to be fixed before landing unless you feel strongly about not adding " is True".
Comment 4 David Kilzer (:ddkilzer) 2018-01-23 11:31:25 PST
Committed r227427: <https://trac.webkit.org/changeset/227427>