[webkit-reviews] review granted: [Bug 32538] [check-webkit-style] Add support for TAB check against text files : [Attachment 44956] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted TAMURA, Kent
<tkent at chromium.org>'s request for review:
Bug 32538: [check-webkit-style] Add support for TAB check against text files
https://bugs.webkit.org/show_bug.cgi?id=32538

Attachment 44956: Proposed patch (rev.3)
https://bugs.webkit.org/attachment.cgi?id=44956&action=review

------- Additional Comments from David Levin <levin at chromium.org>
r+ *if* text_style.can_handle is changed to return false for files in the
LayoutTests directory.

Also, please consider adding your new unit test files to
WebKitTools/Scripts/run-webkit-unittests and fixing the nits below.

Chris made a number of good points that would be nice to address in a future
patch.

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +	   * Scripts/modules/text_style.py:
> +	     Added. This is a liner module for generic text files. It supports

typo: liner


> diff --git a/WebKitTools/Scripts/modules/style.py
b/WebKitTools/Scripts/modules/style.py
> +# FIXME: Move more code from cpp_style to here.
> +# check-webkit-style sholud not refer cpp_style directly.

typo: sholud


> +def parse_arguments(args, additional_flags=[], display_help=False):
> +    """Parses the command line arguments.
> +
> +    See cpp_style.parse_arguments() for detail.


s/detail/details/



> diff --git a/WebKitTools/Scripts/modules/text_style.py
b/WebKitTools/Scripts/modules/text_style.py
> +"""Does WebKit-lint on text files.
> +
> +This modules shares error count, filter setting, output setting, etc. with
cpp_style.

s/modules/module/


> +def process_file_data(filename, lines, error):
> +    """Performs lint check for text on the specified lines.
> +
> +    It reports erros to the given error function.

typo: erros


> +def process_file(filename, error=cpp_style.error):
> +    """Performs lint check for text on a single file."""
> +    try:
> +	   # Do not suuprot for filename='-. cpp_style handles it.

typo: suuprot


> +def can_handle(filename, extension):
> +    """Checks if this module supports for the specified file type.

remove "for"

> +    return ("ChangeLog" in filename
> +	       or "WebKitTools/Scripts/" in filename
> +	       or extension in ('.css', '.html', '.idl', '.js', '.mm', '.php',
'.pm', '.py', '.txt'))

Please make this *not handle* files in the LayoutTest directory. TABs, while
infrequent, are allowed in js and html files there. (Tabs in the files there
ensure that the js and html parsing handle them correctly.)