[webkit-reviews] review denied: [Bug 27549] Make run-webkit-lint supports files as arguments : [Attachment 33274] Make run-webkit-lint supports files as arguments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kenneth Rohde Christiansen
<kenneth.christiansen at openbossa.org>'s request for review:
Bug 27549: Make run-webkit-lint supports files as arguments
https://bugs.webkit.org/show_bug.cgi?id=27549

Attachment 33274: Make run-webkit-lint supports files as arguments
https://bugs.webkit.org/attachment.cgi?id=33274&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Thanks for the patch.  This is a nice addition.

Just a few things to address at the moment.

Well, first off, sorry, I changed the name of the tool on you. So if you would
switch the patch over to check-webkit-style, that would be excellent.

fwiw, you don't need to request me as the reviewer - Adam has been also
reviewing these and doing a great job (The funny thing is that it delayed your
review because I typically scan the general queue -- not used to being asked
for by name). cc'ing me did get my attention though :)


> diff --git a/WebKitTools/Scripts/run-webkit-lint
b/WebKitTools/Scripts/run-webkit-lint

Please fix up "cpplint._USAGE" to reflect the fact that you may optionally pass
in files.

What should happen if the user passes in --git-commit and files?  (Personally I
recommend something simple like an error message that you need to choose one or
the other.)

> +    if not filenames:
> +	   cwd = os.path.abspath('.')
> +	   scm = detect_scm_system(cwd)
> +
> +	   process_patch(scm.create_patch())
> +    else:
> +	   # Change stderr to write with replacement characters so we don't die

> +	   # if we try to print something containing non-ASCII characters.
> +	   sys.stderr = codecs.StreamReaderWriter(sys.stderr,
> +						  codecs.getreader('utf8'),
> +						  codecs.getwriter('utf8'),
> +						  'replace')

If this is useful (and I think it is), it seems like it would be needed for
both code paths, so I'd move this outside of the "if".

> +
> +	   for filename in filenames:
> +	       cpplint.process_file(filename)