| Summary: | Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
| Component: | New Bugs | Assignee: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 64385 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dimitri Glazkov (Google)
2011-07-12 13:24:42 PDT
Created attachment 100553 [details]
WIP: Needs tests.
Created attachment 100575 [details]
With tests.
Comment on attachment 100575 [details]
With tests.
this is wrong. fixing.
Created attachment 100579 [details]
Much better.
Comment on attachment 100579 [details] Much better. 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. Comment on attachment 100579 [details] Much better. View in context: https://bugs.webkit.org/attachment.cgi?id=100579&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:152 > + result.append(expectation.comment) See comment, below. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:171 > + [[<modifiers>] : <name> = <expectations>][ //<comment>] Historically, comments are only allowed on a line by themselves. Do you mean to change this? I don't know that I see a real advantage to supporting it, but I don't feel strongly about it either way. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:173 > + Any errant whitespace is not preserved. Do you wish to preserve blank lines? If you don't, round-tripping the file might make the file a lot harder for people to read. >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208 >> + comment = line[comment_index + 2:] > > You can't split on substrings in python? line, comment_part = [x.strip() for x in line.split('//')] ? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:247 > +class TestExpectationsFile: I wouldn't use the name "File" here, if it's not actually pointing to a file (and it doesn't have to point to a file). Maybe Sequence or List? Also, we used to have a TestExpectationsFile class that was basically the whole giant thing; calling it this might confuse the old-timers used to the earlier code. Comment on attachment 100579 [details] Much better. View in context: https://bugs.webkit.org/attachment.cgi?id=100579&action=review >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:171 >> + [[<modifiers>] : <name> = <expectations>][ //<comment>] > > Historically, comments are only allowed on a line by themselves. Do you mean to change this? I don't know that I see a real advantage to supporting it, but I don't feel strongly about it either way. No, that was existing behavior. There are actually a couple of uses of this syntax in test_expectations today. I was really tempted to just get rid of them, but decided against it to minimize change. >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:173 >> + Any errant whitespace is not preserved. > > Do you wish to preserve blank lines? If you don't, round-tripping the file might make the file a lot harder for people to read. Blank lines are preserved. In fact, line = TestExpectationLine() produces a blank line :) The errant whitespace is the extra spacing around modifiers, name, expectations, and comment (see the test_string_whitespace_stripping unit test for examples) >>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208 >>> + comment = line[comment_index + 2:] >> >> You can't split on substrings in python? > > line, comment_part = [x.strip() for x in line.split('//')] ? D'oi! :) I'll fix that up. Thanks, Python gurus! >> 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. Yeah, I'll rework this. >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:233 >> +class TestExpectationLine: > > Seems like _split_expectation_string could unpack into TestExpectationLine directly. Yep. >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:247 >> +class TestExpectationsFile: > > I wouldn't use the name "File" here, if it's not actually pointing to a file (and it doesn't have to point to a file). Maybe Sequence or List? > > Also, we used to have a TestExpectationsFile class that was basically the whole giant thing; calling it this might confuse the old-timers used to the earlier code. TestExpectationsList sounds good. Comment on attachment 100579 [details] Much better. View in context: https://bugs.webkit.org/attachment.cgi?id=100579&action=review >>>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208 >>>> + comment = line[comment_index + 2:] >>> >>> You can't split on substrings in python? >> >> line, comment_part = [x.strip() for x in line.split('//')] ? > > D'oi! :) I'll fix that up. Thanks, Python gurus! Ah, turns out it's not that easy. We should allow for a comment consisting of eight slashes, for example. I am gonna land this and polish with follow-up patches. Committed r90920: <http://trac.webkit.org/changeset/90920> |