The forthcoming patch adds the ability to check-webkit-style scan directories instead of single files/file lists.
Created attachment 49203 [details] Add the ability to check directories in addition to a list of files
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 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.
(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))
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
(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.
Created attachment 51849 [details] Proposed patch Submitting a patch because this is useful for something I'll be working on soon.
Comment on attachment 51849 [details] Proposed patch Looks good. Thanks!
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
Thanks for the weekend review! Manually committed after rebasing and resolving the conflict: http://trac.webkit.org/changeset/56690
(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