Bug 35234

Summary: check-webkit-style: Make it possible to scan directories
Product: WebKit Reporter: Leandro Pereira <leandro>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Enhancement CC: cjerdonek, commit-queue, eric, hamaji, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Add the ability to check directories in addition to a list of files
hamaji: review-, eric: commit-queue-
Proposed patch hamaji: review+, commit-queue: commit-queue-

Description Leandro Pereira 2010-02-22 04:42:43 PST
The forthcoming patch adds the ability to check-webkit-style scan directories instead of single files/file lists.
Comment 1 Leandro Pereira 2010-02-22 04:45:05 PST
Created attachment 49203 [details]
Add the ability to check directories in addition to a list of files
Comment 2 Eric Seidel (no email) 2010-02-22 14:37:30 PST
Comment on attachment 49203 [details]
Add the ability to check directories in addition to a list of files

The purple bubbles mean that this patch fails to apply. Click on any one of them to see the error messages.  If it fails to apply it can't be commit-queue'd.
Comment 3 Shinichiro Hamaji 2010-02-23 00:34:29 PST
Comment on attachment 49203 [details]
Add the ability to check directories in addition to a list of files

Thanks for this patch! I think this feature is useful. r- for style issues below.

> +        """ Returns a list of files from path, recursively. """

Unnecessary whitespace between """ and Returns. By the way, I think we don't need this function. Please see the comment below.

> +        l = []

Please use more descriptive name. Maybe file_list?

> +    def check_files(self, files, *args, **kwargs):

I'm not sure why we need *args and **kwargs.

It would be better to rename "files" to "file_paths" or something like this.

> +        """Check style in the given files. If files contains a directory,
> +        check it recursively.

The first line of docstrings should be one-line summary. Maybe

"""Check style in the given files. If files contains a directory,

check it recursively.
...


See http://www.python.org/dev/peps/pep-0257/ for detail.

> +          file_path: A list of strings that contains the the path of the
> +                     files to process.

The name is inconsistent with the actual parameter name.

> +        for filename in files:

I'd like to renmae filename to file_path or something.

> +            if os.path.isdir(filename):
> +                self.check_files(self._listdir_recursive(filename), *args, **kwargs)

I believe we don't need _listdir_recursive.

self.check_files([os.path.join(filename, p) for p in os.listdir(filename)])

should work (again, we need better name).

Also this line has more than 80 characters. Recently, we are trying to avoid >80 cols lines.


I think we need a unittest for check_files. You may be able to mock check_file function to test check_files.
Comment 4 Chris Jerdonek 2010-02-23 13:59:04 PST
(In reply to comment #1)
> Created an attachment (id=49203) [details]
> Add the ability to check directories in addition to a list of files

You may want to consider a simpler, non-recursive approach using os.walk.

For example, I came across the following method in pep8.py (which we will be using) which seems to do the same thing:

def input_dir(dirname):
    """
    Check all Python source files in this directory and all subdirectories.
    """
    dirname = dirname.rstrip('/')
    if excluded(dirname):
        return
    for root, dirs, files in os.walk(dirname):
        if options.verbose:
            message('directory ' + root)
        options.counters['directories'] = \
            options.counters.get('directories', 0) + 1
        dirs.sort()
        for subdir in dirs:
            if excluded(subdir):
                dirs.remove(subdir)
        files.sort()
        for filename in files:
            if filename_match(filename):
                input_file(os.path.join(root, filename))
Comment 5 Chris Jerdonek 2010-02-24 08:18:09 PST
Retitling.  Brackets are usually reserved for port-specific reports like [Qt}, [GTK], etc and not feature-specific reports.  See here, for example--

https://lists.webkit.org/pipermail/webkit-dev/2010-January/011374.html
Comment 6 Leandro Pereira 2010-02-24 09:35:18 PST
(In reply to comment #4)
> 
> You may want to consider a simpler, non-recursive approach using os.walk.
>

Nice. I only knew about os.path.walk, which requires some additional code. I'll rewrite my code to use os.walk, then.
Comment 7 Chris Jerdonek 2010-03-27 20:35:15 PDT
Created attachment 51849 [details]
Proposed patch

Submitting a patch because this is useful for something I'll be working on soon.
Comment 8 Shinichiro Hamaji 2010-03-28 04:26:56 PDT
Comment on attachment 51849 [details]
Proposed patch

Looks good. Thanks!
Comment 9 WebKit Commit Bot 2010-03-28 04:37:53 PDT
Comment on attachment 51849 [details]
Proposed patch

Rejecting patch 51849 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Shinichiro Hamaji', '--force']" exit_code: 1
Last 500 characters of output:
ebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/Scripts/check-webkit-style
Hunk #1 FAILED at 87.
1 out of 1 hunk FAILED -- saving rejects to file WebKitTools/Scripts/check-webkit-style.rej
patching file WebKitTools/Scripts/webkitpy/style/checker.py
patching file WebKitTools/Scripts/webkitpy/style/checker_unittest.py
Hunk #2 succeeded at 647 (offset 2 lines).
patching file WebKitTools/Scripts/webkitpy/style/optparser.py
Hunk #1 succeeded at 42 (offset 3 lines).

Full output: http://webkit-commit-queue.appspot.com/results/1626033
Comment 10 Chris Jerdonek 2010-03-28 07:53:35 PDT
Thanks for the weekend review!

Manually committed after rebasing and resolving the conflict:

http://trac.webkit.org/changeset/56690
Comment 11 Chris Jerdonek 2010-03-28 08:06:56 PDT
(In reply to comment #10)
> Thanks for the weekend review!
> 
> Manually committed after rebasing and resolving the conflict:
> 
> http://trac.webkit.org/changeset/56690

Fixed this commit:

http://trac.webkit.org/changeset/56691