Bug 35234 – check-webkit-style: Make it possible to scan directories

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Leandro Pereira]

The forthcoming patch adds the ability to check-webkit-style scan directories instead of single files/file lists.

[Leandro Pereira]

Created attachment 49203 [details]
Add the ability to check directories in addition to a list of files

[Eric Seidel (no email)]

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.

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

(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))

[Chris Jerdonek]

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

[Leandro Pereira]

(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.

[Chris Jerdonek]

Created attachment 51849 [details]
Proposed patch

Submitting a patch because this is useful for something I'll be working on soon.

[Shinichiro Hamaji]

Comment on attachment 51849 [details]
Proposed patch

Looks good. Thanks!

[WebKit Commit Bot]

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

[Chris Jerdonek]

Thanks for the weekend review!

Manually committed after rebasing and resolving the conflict:

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

[Chris Jerdonek]

(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