[Webkit-unassigned] [Bug 32538] [check-webkit-style] Add support for TAB check against text files

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Chris Jerdonek <chris.jerdonek at gmail.com>  2009-12-16 09:29:00 PST ---
(In reply to comment #12)
> Created an attachment (id=44956)
 --> (https://bugs.webkit.org/attachment.cgi?id=44956) [details]
> Proposed patch (rev.3)

Looking a lot better, thanks!

The check-webkit-style doc string should probably be updated:

> """Does WebKit-lint on C/C++ files.

I think the code will be simpler if this just takes a filename:

> +def can_handle(filename, extension):

The calling code won't have to split out the file extension (I count this being
done four times in various calling code):

> file_extension = os.path.splitext(filename)[1]

The unit tests will be simpler:

> +        self.assert_(text_style.can_handle('foo.css', '.css'))
> +        self.assert_(text_style.can_handle('foo.html', '.html'))
> +        self.assert_(text_style.can_handle('foo.idl', '.idl'))

Also, the second parameter is redundant data and introduces the possible error
case of the given extension not matching the true extension.

> +++ b/WebKitTools/Scripts/modules/style.py
> @@ -0,0 +1,101 @@
> +# Copyright (C) 2009 Google Inc. All rights reserved.
> +#

Does Apple copyright need to be added above and in other new files?  I'm not
sure.

> +    It reports erros to the given error function.

errors

> +"""Frontend of some style-checker modules."""

Front end

> +    if cpp_style.can_handle(filename, file_extension) or filename == '-':
> +        cpp_style.process_file(filename)
> +    elif text_style.can_handle(filename, file_extension):
> +        text_style.process_file(filename)

It might be worth adding in the doc string of the *.process_file methods that
they still process the file even if passed a filename they can't "handle". 
That way future developers don't add parameter checking code at the top of the
*.process_file implementations that call can_handle and prematurely exit if
they don't.

Some might argue that process_file should prematurely exit.  But for testing
purposes, etc, I do think process_file should attempt to process all files as
you are currently doing.

> +def process_file_data(filename, lines, error):

> +        if '\t' in line:
> +            error(filename, line_number, 'whitespace/tab', 5, 'Line contains tab character.')

This is nearly identical to the whitespace/tab checking code at the beginning
of cpp_style.check_style:

>    if line.find('\t') != -1:
>        error(filename, line_number, 'whitespace/tab', 1,
>              'Tab found; better to use spaces')

It might be good to define a check_whitespace_tab() method in cpp_style and
have text_style call that one.

I anticipate that there will be a lot of overlap between the style-checking
rules of various file types.  So it would be good if we can begin to establish
a pattern of sharing the code for those checks where possible.

> +        # Do not suuprot for filename='-. cpp_style handles it.

Not sure what this is trying to say -- also typo in "support".

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