[webkit-reviews] review denied: [Bug 43899] Add test_expectations.txt syntax checker to check-webkit-style : [Attachment 64193] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 12 02:34:57 PDT 2010
Shinichiro Hamaji <hamaji at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 43899: Add test_expectations.txt syntax checker to check-webkit-style
https://bugs.webkit.org/show_bug.cgi?id=43899
Attachment 64193: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=64193&action=review
------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Thanks for doing this! I believe this is very useful. Putting r- for nitpicks.
WebKitTools/Scripts/webkitpy/style/checker.py:42
+ from checkers.test_expectations import TestExpectationsChecker
It would be better to put this line above checkers.text as we are usually put
#include in alphabetical order in C++ code.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:40
+ sys.path.append(os.path.abspath(os.path.join(d, '../../layout_tests')))
Cannot we just do like
from webkitpy.layout_tests.layout_package import test_expectations
?
Also, I think it would be nice if you add these imports into
webkitpy/style_reference.py
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48
+ class OptionsMock(object):
Why do we need this? Could you add some comments into this docstring?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48
+ class OptionsMock(object):
I think the name "Mock" is usually used in unittests. How about
ChromiumOptions?
WebKitTools/Scripts/webkitpy/style/checker.py:450
+ if os.path.basename(file_path) == 'test_expectations.txt':
Can we still warn tabs in test_expectations.txt ?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:93
+ errors = str(err).split('\n')
Not sure, but we can use str.splitlines?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:85
+ exp = None
Abbreviation isn't encouraged in WebKit. I'd say expectations.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:63
+ self._output_regex = re.compile('Line:(\d+)\s*(.+)')
It would be better to use named sub-group. Like,
re.compile('Line:(?P<line>\d+)\s*(?P<message>.+)')
With this, you can write match.group('line') to get \d+
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:96
+ match = self._output_regex.match(err)
s/match/matched/ as match is verb and matched is more consistent with cpp.py
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:92
+ except SyntaxError, err:
As we will use "err" for different value, I'd call this as error_object or
something line this.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:95
+ for err in errors:
I slightly prefer error
WebKitTools/ChangeLog:8
+ Just utiizing layout_tests/layout_package/test_expectations.py for
checking
utilizing
More information about the webkit-reviews
mailing list