Add lint_filters parameter to CheckChangeLintsClean function.

Add lint_filters parameter to CheckChangeLintsClean function.

The idea here is that when one of the filters are cleaned up in Chromium,
they should not be filtered out, otherwise we won't catch them in
the presubmit step.

See for example -> https://codereview.chromium.org/788493002/

BUG=None
R=agable@chromium.org, dpranke@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=293357

[tfarina, 2014-12-08 01:05:39]

Dirk, feel free to suggest a better way we could do this.

For context, you can read https://codereview.chromium.org/788493002/ to see
where this is coming from.

Eventually, I want to make //ui/* lint clean, app_list was just the first step.

Thanks,

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:109: # - build/include       : Too many; fix in the
future.
I want to enable these checks in app_list, as we cleaned them there, and to
avoid future regressions. But as we are filtering them out here, we are always
regressing this.

[tfarina, 2014-12-09 17:39:16]

Friendly ping?

Feel free to suggest another reviewer though.

[Dirk Pranke, 2014-12-09 21:17:07]

+thakis ...

Nico, maybe you have thoughts on this?

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:109: # - build/include       : Too many; fix in the
future.
On 2014/12/08 01:05:39, tfarina wrote:
> I want to enable these checks in app_list, as we cleaned them there, and to
> avoid future regressions. But as we are filtering them out here, we are always
> regressing this.

It looks like ui/app_list/PRESUBMIT.py has its own copy of
CheckChangeLintsClean() and maybe only src/chrome calls this function;

perhaps we should just remove this function and move it to src/chrome if no one
else uses it and we think we might want different settings per component?

Apart from that, idiomatically, I'd probably expose the default list on lines
118-120 as a global constant, to allow callers to get the default set and modify
it, rather than forcing them to duplicate things.

Then I'd just change the start of this function to add on line 96 'lint_filters
= lint_filters or DEFAULT_LINT_FILTERS' and get rid of this branch.

[agable, 2014-12-09 21:34:48]

agable@chromium.org changed reviewers:
+ agable@chromium.org

[agable, 2014-12-09 21:34:49]

LGTM % nit.

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:104: if lint_filters != None:
s/!=/is not/

[Nico, 2014-12-09 22:53:39]

On 2014/12/09 21:17:07, Dirk Pranke wrote:
> +thakis ...
> 
> Nico, maybe you have thoughts on this?

Not really. What you write below ("if this used only in chromium, move it to
chromium") makes sense to me though.

I do have one thought: presubmits are already too slow, we should only add new
ones if they're fast (or only add new ones if we remove others instead).

> https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
> 
>
https://codereview.chromium.org/779033003/diff/1/presubmit_canned_checks.py#n...
> presubmit_canned_checks.py:109: # - build/include       : Too many; fix in the
> future.
> On 2014/12/08 01:05:39, tfarina wrote:
> > I want to enable these checks in app_list, as we cleaned them there, and to
> > avoid future regressions. But as we are filtering them out here, we are
always
> > regressing this.
> 
> It looks like ui/app_list/PRESUBMIT.py has its own copy of
> CheckChangeLintsClean() and maybe only src/chrome calls this function;
> 
> perhaps we should just remove this function and move it to src/chrome if no
one
> else uses it and we think we might want different settings per component?
> 
> Apart from that, idiomatically, I'd probably expose the default list on lines
> 118-120 as a global constant, to allow callers to get the default set and
modify
> it, rather than forcing them to duplicate things.
> 
> Then I'd just change the start of this function to add on line 96
'lint_filters
> = lint_filters or DEFAULT_LINT_FILTERS' and get rid of this branch.

[tfarina, 2014-12-10 00:23:36]

Dirk, I tried to implement the way you suggested.

Could you take another look at patch set 2?

Btw, the way I understand it app_list is not implementing its own
CheckChangeLintsClean, well maybe partially, but just to pass a 'correct' list
of C++ files.

Hum, the INCLUDE_CPP_FILES_ONLY, may not be necessarily anymore, since
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/presubm...
does this?

Well, we still need to black_list cocoa (objc) files there, but I haven't get to
that yet.

But this is subject for another CL :)

PTAL.

[tfarina, 2014-12-10 00:36:12]

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:21: DEFAULT_LINT_FILTERS = [
Looks like this is wrong:

File "/home/tfarina/depot_tools/cpplint.py", line 809, in AddFilters
    for filt in filters.split(','):
AttributeError: 'list' object has no attribute 'split'

How DEFAULT_LINT_FILTERS should look like? That is what I don't like very much
on Python. :/

[Dirk Pranke, 2014-12-10 02:13:18]

I guess you're right that app_list was re-using the CheckChangeLintsClean() call
in depot_tools.

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:21: DEFAULT_LINT_FILTERS = [
On 2014/12/10 00:36:11, tfarina wrote:
> Looks like this is wrong:
> 
> File "/home/tfarina/depot_tools/cpplint.py", line 809, in AddFilters
>     for filt in filters.split(','):
> AttributeError: 'list' object has no attribute 'split'
> 
> How DEFAULT_LINT_FILTERS should look like? That is what I don't like very much
> on Python. :/

The original thing was a single comma-separated string, so you need to change
this to:


DEFAULT_LINT_FILTERS = ','.join([
   '-build/include',
   ...
])

[Dirk Pranke, 2014-12-10 02:15:18]

lgtm w/ the constant and the call fixed ...

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:21: DEFAULT_LINT_FILTERS = [
On 2014/12/10 02:13:18, Dirk Pranke wrote:
> On 2014/12/10 00:36:11, tfarina wrote:
> > Looks like this is wrong:
> > 
> > File "/home/tfarina/depot_tools/cpplint.py", line 809, in AddFilters
> >     for filt in filters.split(','):
> > AttributeError: 'list' object has no attribute 'split'
> > 
> > How DEFAULT_LINT_FILTERS should look like? That is what I don't like very
much
> > on Python. :/
> 
> The original thing was a single comma-separated string, so you need to change
> this to:
> 
> 
> DEFAULT_LINT_FILTERS = ','.join([
>    '-build/include',
>    ...
> ])
> 
>  

Er, I mean, leave this as a list, and change the _SetFilters() call to
_SetFilters(','.join(lint_filters)).

That way lint_filters is a list, rather than a comma-separated string.

[tfarina, 2014-12-10 03:40:26]

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/779033003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:21: DEFAULT_LINT_FILTERS = [
On 2014/12/10 02:15:18, Dirk Pranke wrote:
> On 2014/12/10 02:13:18, Dirk Pranke wrote:
> > On 2014/12/10 00:36:11, tfarina wrote:
> > > Looks like this is wrong:
> > > 
> > > File "/home/tfarina/depot_tools/cpplint.py", line 809, in AddFilters
> > >     for filt in filters.split(','):
> > > AttributeError: 'list' object has no attribute 'split'
> > > 
> > > How DEFAULT_LINT_FILTERS should look like? That is what I don't like very
> much
> > > on Python. :/
> > 
> > The original thing was a single comma-separated string, so you need to
change
> > this to:
> > 
> > 
> > DEFAULT_LINT_FILTERS = ','.join([
> >    '-build/include',
> >    ...
> > ])
> > 
> >  
> 
> Er, I mean, leave this as a list, and change the _SetFilters() call to
> _SetFilters(','.join(lint_filters)).
> 
> That way lint_filters is a list, rather than a comma-separated string.

Done.

[tfarina, 2014-12-12 00:03:55]

Committed patchset #3 (id:40001) manually as 293357.