Summary: | check-webkit-style: Files should be ignored unless they have an associated checker | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cjerdonek, commit-queue, hamaji | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Chris Jerdonek
2010-04-27 06:47:07 PDT
This report should address the following two FIXME's: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L404 http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L673 Created attachment 54419 [details]
Proposed patch
Created attachment 54420 [details]
Proposed patch 2
Comment on attachment 54420 [details]
Proposed patch 2
WebKitTools/Scripts/webkitpy/style/checker.py:176
+ 'txt',
Can we still check ChangeLogs? First I thought the whitelist approach sounds OK when I've seen the bug description. However, I've found there are many extensions ideally we might want to check. Maybe we should add some more extensions: 'y', 'wm', 'xhtml', 'cgi', 'in', 'rb', 'pl', 'cc', 'gyp', 'gypi', 'flex', 'sh', 'pri', 'pro', and... there can be more. Hmm... Now I'm not sure if the whitelist approach is the right way...
(In reply to comment #4) > (From update of attachment 54420 [details]) > WebKitTools/Scripts/webkitpy/style/checker.py:176 > + 'txt', > Can we still check ChangeLogs? ChangeLogs should still be getting checked because they have FileType.TEXT. I can add a unit test for this case to be sure. I did notice, however, that LayoutTests/ChangeLog is not getting checked. But I believe that problem was already present. Perhaps we can deal with that special case later. I can add it as a FIXME. > First I thought the whitelist approach sounds OK > when I've seen the bug description. However, I've found there are many > extensions ideally we might want to check. Maybe we should add some more > extensions: 'y', 'wm', 'xhtml', 'cgi', 'in', 'rb', 'pl', 'cc', 'gyp', 'gypi', > 'flex', 'sh', 'pri', 'pro', and... there can be more. Hmm... Now I'm not sure > if the whitelist approach is the right way... Yes, a black list is another approach that would be okay as long as we centralize the logic as we are doing in this report. The black list approach may have the same problem though because we'd still have to list out every possible binary file extension: gif, jpeg, pyc, png, etc. Given that, I think the white list approach may be better because then we don't risk accidentally processing binary files (e.g. when passing explicit directory paths). The only down-side is that, yes, we need to explicitly opt text files in. That could be a good feature though because at least then we can account for all files we are checking. I would be okay with adding more text extensions in this patch if you provide more. > Yes, a black list is another approach that would be okay as long as we
> centralize the logic as we are doing in this report. The black list approach
> may have the same problem though because we'd still have to list out every
> possible binary file extension: gif, jpeg, pyc, png, etc. Given that, I think
> the white list approach may be better because then we don't risk accidentally
> processing binary files (e.g. when passing explicit directory paths). The only
> down-side is that, yes, we need to explicitly opt text files in. That could be
> a good feature though because at least then we can account for all files we are
> checking. I would be okay with adding more text extensions in this patch if
> you provide more.
I see. Thanks for the nice description. I'm totally convinced. Let's go ahead with the whitelist approach. I'd add 'exp' and 'ac' in addition to my previous comment.
Comment on attachment 54420 [details]
Proposed patch 2
The code change looks good. I'm putting r+ assuming you'll add extensions I listed.
WebKitTools/Scripts/webkitpy/style/checker.py:674
+ raise Exception("File should not be checked: '%s'" % file_path)
This shouldn't happen if this function isn't called from unittest, right? If so, how about using assert statement?
WebKitTools/Scripts/webkitpy/style/checker_unittest.py:764
+ self._processor.process(lines=['line1', 'line2'],
If you use assert statement above. We can just use assertRaises(AssertionError, ...) ?
Created attachment 54521 [details]
Proposed patch 3
Incorporated Shinichiro's suggestions. Also added more thorough comments and unit tests, and addressed the issue of LayoutTests/ChangeLog getting skipped.
Comment on attachment 54521 [details]
Proposed patch 3
Looks great. Thanks for your update!
Comment on attachment 54521 [details] Proposed patch 3 Clearing flags on attachment: 54521 Committed r58401: <http://trac.webkit.org/changeset/58401> All reviewed patches have been landed. Closing bug. |