Bug 180820 – [webkitpy] Add ignore_errors keyword argument to Executive.run_command() for replacing the common pattern of error_handler usage

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Basuke Suzuki]

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.

[Basuke Suzuki]

Also there's no need to import Executive module just to pass Executive.ignore_error static method if this option is available.

[Basuke Suzuki]

Created attachment 329373 [details]
patch

[David Kilzer (:ddkilzer)]

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".

[David Kilzer (:ddkilzer)]

Committed r227427: <https://trac.webkit.org/changeset/227427>