Bug 27549 – Make run-webkit-lint supports files as arguments

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

Make run-webkit-lint supports files as arguments so that we dont need to call
python WebKitTools/Scripts/modules/cpplint.py on individual files.

Patch coming up.

[Kenneth Rohde Christiansen]

Created attachment 33274 [details]
Make run-webkit-lint supports files as arguments

[David Levin]

Comment on attachment 33274 [details]
Make run-webkit-lint supports files as arguments

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)

[Kenneth Rohde Christiansen]

Created attachment 33712 [details]
Updates and improved patch

[David Levin]

Comment on attachment 33712 [details]
Updates and improved patch

I'm willing to do these changes, but I'd like to know how you feel about the "consider" comments.


> diff --git a/WebKitTools/Scripts/check-webkit-style b/WebKitTools/Scripts/check-webkit-style
> +  The file parameter is optional and multiple file scan be passed in. Leaving

Consider:
  The file parameter is optional and multiple files to scan may be passed in

> +  out the file parameter will apply the check to the files currently not

Consider:
  out the file parameter will apply the check to the files changed 
  according to the scm system.


> +    if files and "--git-commit" in flags:
> +        sys.stderr.write("ERROR: It is not possible to check files "
> +                          "and a specific commit at the same time.\n" + cpp_style._USAGE)
>          sys.exit(1)
> +    elif files:

Make this an "if" since the pre if "exits".

[Kenneth Rohde Christiansen]

I feel fine about these changes.

[Kenneth Rohde Christiansen]

Comment on attachment 33712 [details]
Updates and improved patch

Landed (with the above changes) in 46542.