[Webkit-unassigned] [Bug 43899] Add test_expectations.txt syntax checker to check-webkit-style

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43899


Shinichiro Hamaji <hamaji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64193|review?                     |review-
               Flag|                            |




--- Comment #3 from Shinichiro Hamaji <hamaji at chromium.org>  2010-08-12 02:34:58 PST ---
(From update of attachment 64193)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.