[Webkit-unassigned] [Bug 35234] [check-webkit-style] Make it possible to scan directories

[bugzilla-daemon at webkit.org]

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


Shinichiro Hamaji <hamaji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49203|review?                     |review-
               Flag|                            |




--- Comment #3 from Shinichiro Hamaji <hamaji at chromium.org>  2010-02-23 00:34:29 PST ---
(From update of attachment 49203)
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.

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