[Webkit-unassigned] [Bug 27291] Support lint for patches

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Shinichiro Hamaji <hamaji at chromium.org>  2009-07-15 09:00:46 PDT ---
Thanks for the comments, I'll change the order of your comments.

> *** General comment ***
> For the rest of this, it feels like the use case should be someone who has the
> patch in their scm. The user would be to run the lint tool on the patch that
> would be created.
> 
> With that in mind could this change would do the following: 
> 1. Getting the current diff from the scm (use "create_patch" in scm.py.  A
> subsequent change can add support to call create_patch_from_local_commit for
> git users.)
> 2. Run cpplint on the files in that diff.
> 3. Filter the output from cpplint to only include the lines that changed in
> that diff.
> 
> Also, this logic could reside outside of cpplint.py, there could be a
> run-webkit-lint that did most of this and then called cpplint to do the linting
> of cpp/h files (with what lines to filter the results to).
> 
> I like this approach because you'll get less false lint warnings from running
> it on the whole file and it feels like it fits the usage model.  (If I want to
> lint someone else's patch, I could always patch it into my local system and
> then run the lint tool on it.) 

I see. I considered we may eventually want to integrate the linter into
bugzilla so that submitted patch will be automatically linted and reviewees may
see style issues even if they forget to apply the linter. Anyway, it's
reasonable to implement scm integration first.

> This code may not be used in WebKit because it has an incompatible license:
> "Licensed under the Apache License"
> 
> By the way, this is the typical license for Google code. (cpplint.py was
> relicense due to Albert Wong asking the author so that it could be used in
> WebKit.)
> 
> So there are two options:
> 1. See if the code can be relicensed (or dual licensed) (which may not be easy
> to do if people outside of Google contributed) OR
> 2. Write something equivalent from scratch.

The approach you suggested would be easier than the original way (we don't need
to have equivalent of parse_patch_to_chunks, which is the biggest function in
my patch). So, it may be not so difficult to re-invent other parts. Do you
think it's reasonable to re-implement them? Or, should we ask the authors of
rietveld about the license first?

> As you do this work, don't feel like you need a fully completely functional
> thing to submit a patch.  Just take logical chunks that are complete by
> themselves but may not provide all the functionality and submit patches as you
> go along so that the patches stay small which will make the review process
> faster.

OK, I'll split my patch into small pieces. I know big patches are not good, but
I thought it would be OK in this case as this patch was mostly copy&paste and
unittest.

> For example one patch, may be just to move cpplint.py (and its unit test) into
> Scripts/modules since we'll probably call it from "run-webkit-lint" (which
> would be the harness you'll write in these patches).

Will do.

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