Makefiles will always contain tabs so make the notab check filter them out.

Makefiles will always contain tabs so make the notab check filter them out.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50251

[M-A Ruel, 2010-06-18 00:36:46]

I can't type...

[petkov, 2010-06-18 03:55:15]

Thanks for looking into this -- the Makefile tab warnings were pretty annoying.

FWIW, LGTM (I can't claim to completely understand how all filtering and default
filtering works).

A unit test (that Makefiles are excluded) might have been nice but you're the
owner so your call.

http://codereview.chromium.org/2814017/diff/1/2
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/2814017/diff/1/2#newcode221
presubmit_canned_checks.py:221: return (not
input_api.re.match(r'(^|[\\\/])(M|m)akefile$', filepath) and
I copied that part of the regex and I wasn't sure: shouldn't [\\/] be OK (one
less \)?

Also,

re.match(r'^(M|m)akefile$', os.path.basename(filepath))

may be a bit more robust across platforms.

[M-A Ruel, 2010-06-18 13:49:50]

On 2010/06/18 03:55:15, petkov wrote:
> Thanks for looking into this -- the Makefile tab warnings were pretty
annoying.

Too bad nobody notified me earlier...


> FWIW, LGTM (I can't claim to completely understand how all filtering and
default
> filtering works).

Yeah, I'd like it to be simpler.


> A unit test (that Makefiles are excluded) might have been nice but you're the
> owner so your call.

You were right, I added one and the code was in fact broken. Fixed.


> http://codereview.chromium.org/2814017/diff/1/2
> File presubmit_canned_checks.py (right):
> 
> http://codereview.chromium.org/2814017/diff/1/2#newcode221
> presubmit_canned_checks.py:221: return (not
> input_api.re.match(r'(^|[\\\/])(M|m)akefile$', filepath) and
> I copied that part of the regex and I wasn't sure: shouldn't [\\/] be OK (one
> less \)?

It's usually needed. Maybe not in this particular case since it's not surrounded
by // but it's more for brain consistency.

> Also,
> 
> re.match(r'^(M|m)akefile$', os.path.basename(filepath))

I removed the regexp altogether. Note that it trigger for makefile.foo and the
likes, I'm thinking about makefile.am. Please tell me if you want different
behavior.

M-A

[petkov, 2010-06-18 17:03:40]

LGTM, assuming the unit tests pass.

> I removed the regexp altogether. Note that it trigger for makefile.foo and the
> likes, I'm thinking about makefile.am. Please tell me if you want different
> behavior.

I think what you have now is OK. If we need more, we can address later (or
through local PRESUBMIT.py changes). E.g., I see people using "common.mk"...

Thanks!