[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





--- Comment #6 from Kenichi Ishibashi <bashi at google.com>  2010-08-12 17:29:50 PST ---
(In reply to comment #3)
Hamaji-san,

Thank you for your quick and detailed review! Your comment definitely helpful for me. I'll send the revised patch later.

> (From update of attachment 64193 [details])
> 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.