[webkit-reviews] review denied: [Bug 35234] [check-webkit-style] Make it possible to scan directories : [Attachment 49203] Add the ability to check directories in addition to a list of files

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Leandro Pereira
<leandro at profusion.mobi>'s request for review:
Bug 35234: [check-webkit-style] Make it possible to scan directories
https://bugs.webkit.org/show_bug.cgi?id=35234

Attachment 49203: Add the ability to check directories in addition to a list of
files
https://bugs.webkit.org/attachment.cgi?id=49203&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
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.