Add exception for long url(..) lines in css files in presubmit_canned_checks.CheckLongLines

Add exception for long url(..) lines in css files in presubmit_canned_checks.CheckLongLines

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

[Marc Treib, 2014-07-28 12:16:30]

PTAL!

[M-A Ruel, 2014-07-28 13:43:32]

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

https://codereview.chromium.org/424733003/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:361: if (any((url in line) for url in ('file://',
'http://', 'https://')) or
It's really two separate checks so I'd split it two conditions to make it more
readable.

[Marc Treib, 2014-07-28 13:57:12]

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

https://codereview.chromium.org/424733003/diff/1/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:361: if (any((url in line) for url in ('file://',
'http://', 'https://')) or
On 2014/07/28 13:43:31, M-A Ruel wrote:
> It's really two separate checks so I'd split it two conditions to make it more
> readable.

Done.

[M-A Ruel, 2014-07-28 14:09:05]

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

https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:364: if ('url(' in line) and file_extension == 'css':
if 'url(' in line and file_extension == 'css':

https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:367: return
input_api.re.match(r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol,
I highly prefer:
return input_api.re.match(
    r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line)

The rationale is that if you ever rename the function call (let's say input_api
is renamed to foo_bar), then doing a sed like replace doesn't need the argument
list to be realigned, which saves a lot of time. When you align at (, it makes
refactoring much more painful by requiring a lot of stupid whitespace alignment,
which is a total waste of time.

[Marc Treib, 2014-07-28 14:13:48]

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

https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:364: if ('url(' in line) and file_extension == 'css':
On 2014/07/28 14:09:05, M-A Ruel wrote:
> if 'url(' in line and file_extension == 'css':

Done.

https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
presubmit_canned_checks.py:367: return
input_api.re.match(r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol,
On 2014/07/28 14:09:05, M-A Ruel wrote:
> I highly prefer:
> return input_api.re.match(
>     r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line)
> 
> The rationale is that if you ever rename the function call (let's say
input_api
> is renamed to foo_bar), then doing a sed like replace doesn't need the
argument
> list to be realigned, which saves a lot of time. When you align at (, it makes
> refactoring much more painful by requiring a lot of stupid whitespace
alignment,
> which is a total waste of time.

Done.

[M-A Ruel, 2014-07-28 14:20:11]

On 2014/07/28 14:13:48, treib wrote:
>
https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
> presubmit_canned_checks.py:367: return
> input_api.re.match(r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol,
> On 2014/07/28 14:09:05, M-A Ruel wrote:
> > I highly prefer:
> > return input_api.re.match(
> >     r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line)
> > 
> > The rationale is that if you ever rename the function call (let's say
> input_api
> > is renamed to foo_bar), then doing a sed like replace doesn't need the
> argument
> > list to be realigned, which saves a lot of time. When you align at (, it
makes
> > refactoring much more painful by requiring a lot of stupid whitespace
> alignment,
> > which is a total waste of time.

Wow, re-reading myself, that was harsh, I really need my second coffee. Sorry
for the attitude and thanks for doing it.

lgtm

[M-A Ruel, 2014-07-28 14:20:14]

The CQ bit was checked by maruel@chromium.org

[commit-bot: I haz the power, 2014-07-28 14:20:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/424733003/40001

[commit-bot: I haz the power, 2014-07-28 14:23:13]

Change committed as 285901

[Marc Treib, 2014-07-28 15:06:14]

On 2014/07/28 14:20:11, M-A Ruel wrote:
> On 2014/07/28 14:13:48, treib wrote:
> >
>
https://codereview.chromium.org/424733003/diff/20001/presubmit_canned_checks....
> > presubmit_canned_checks.py:367: return
> > input_api.re.match(r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol,
> > On 2014/07/28 14:09:05, M-A Ruel wrote:
> > > I highly prefer:
> > > return input_api.re.match(
> > >     r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line)
> > > 
> > > The rationale is that if you ever rename the function call (let's say
> > input_api
> > > is renamed to foo_bar), then doing a sed like replace doesn't need the
> > argument
> > > list to be realigned, which saves a lot of time. When you align at (, it
> makes
> > > refactoring much more painful by requiring a lot of stupid whitespace
> > alignment,
> > > which is a total waste of time.
> 
> Wow, re-reading myself, that was harsh, I really need my second coffee. Sorry
> for the attitude and thanks for doing it.
> 
> lgtm

No hard feelings :)