| Summary: | nrwt: remove fs refs from printing, test_failures, test_expectations, text_diff | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | eric, mihaip, ojan, tony | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | 52754 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Dirk Pranke
2011-01-19 15:29:28 PST
Created attachment 79502 [details]
Patch
Comment on attachment 79502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79502&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:126 > - return os.path.splitext(filename)[0] + modifier > + # FIXME: technically this breaks if files don't use ".ext" to indicate > + # the extension, but passing in a Filesystem object here is a huge > + # hassle. > + return filename[filename.rfind('.')] + modifier I might have just left this using os.path.splitext with a FIXME about using filesystem instead. > Tools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:-36 > -from __future__ import with_statement We're sure this isn't needed? Comment on attachment 79502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79502&action=review >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:126 >> + return filename[filename.rfind('.')] + modifier > > I might have just left this using os.path.splitext with a FIXME about using filesystem instead. Does this need testing? Committed r76288: <http://trac.webkit.org/changeset/76288> In reply to comment #2) > (From update of attachment 79502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79502&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:126 > > - return os.path.splitext(filename)[0] + modifier > > + # FIXME: technically this breaks if files don't use ".ext" to indicate > > + # the extension, but passing in a Filesystem object here is a huge > > + # hassle. > > + return filename[filename.rfind('.')] + modifier > > I might have just left this using os.path.splitext with a FIXME about using filesystem instead. > I felt it was better to not have any reference to os.path at all. > > Tools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:-36 > > -from __future__ import with_statement > > We're sure this isn't needed? Yes. There are no "with" statements in the file. (In reply to comment #3) > (From update of attachment 79502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79502&action=review > > >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:126 > >> + return filename[filename.rfind('.')] + modifier > > > > I might have just left this using os.path.splitext with a FIXME about using filesystem instead. > > Does this need testing? It is covered by the existing tests. (In reply to comment #6) > (In reply to comment #3) > > (From update of attachment 79502 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=79502&action=review > > > > >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:126 > > >> + return filename[filename.rfind('.')] + modifier > > > > > > I might have just left this using os.path.splitext with a FIXME about using filesystem instead. > > > > Does this need testing? > > It is covered by the existing tests. Or so I thought :( . See bug 52854. |