[webkit-reviews] review granted: [Bug 64386] Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations. : [Attachment 100579] Much better.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 64386: Extract model-like TestExpectationLine and TestExpectationFile from
TestExpectations.
https://bugs.webkit.org/show_bug.cgi?id=64386

Attachment 100579: Much better.
https://bugs.webkit.org/attachment.cgi?id=100579&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100579&action=review


This looks fantastic.  Some detail nits below.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208
> +	   comment_index = line.find("//")
> +	   comment = ''
> +	   if comment_index == -1:
> +	       comment_index = len(line)
> +	       comment = None
> +	   else:
> +	       comment = line[comment_index + 2:]

You can't split on substrings in python?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:217
> +	       return (None, None, None, None)

Consider using a dictionary instead of a tuple.  That's more self-documenting /
self-error-correcting.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:233
> +class TestExpectationLine:

Seems like _split_expectation_string could unpack into TestExpectationLine
directly.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:260
> +	       (expectation, errors) = TestExpectationParser.parse(line)

Note: You don't need the ( or ) when making a destructuring assignment, which
you might view as prettier.