Remove presubmit warning for long lines in .grd files

Remove presubmit warning for long lines in .grd files

The presubmit check complained about long lines in .grd files. This isn't a useful warning since .grd files aren't really source files.

The problem was that callers were passing in a file filter to InputApi.AffectedFiles but it was being ignored. Fix was to use the passed in file filter.

BUG=None
TEST=Ran on a local change and verified that I no longer got long line warnings on .grd files.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85148

[nsylvain, 2011-05-06 00:41:39]

marc-antoine knows more than me here.

[Dirk Pranke, 2011-05-06 01:03:26]

How was this working at all if the caller was passing in a file_filter argument,
given that that wasn't previously a parameter?

[sail, 2011-05-06 01:15:37]

On 2011/05/06 01:03:26, Dirk Pranke wrote:
> How was this working at all if the caller was passing in a file_filter
argument,
> given that that wasn't previously a parameter?

Callers don't seem to be supplying the include_dirs argument. So the
include_dirs gets set to the filter argument.

[Dirk Pranke, 2011-05-06 03:21:00]

On 2011/05/06 01:15:37, sail wrote:
> On 2011/05/06 01:03:26, Dirk Pranke wrote:
> > How was this working at all if the caller was passing in a file_filter
> argument,
> > given that that wasn't previously a parameter?
> 
> Callers don't seem to be supplying the include_dirs argument. So the
> include_dirs gets set to the filter argument.

Okay.

LGTM.

[commit-bot: I haz the power, 2011-05-06 03:31:37]

Presubmit check for 6932060-1 failed and returned exit status 1.

Running presubmit commit checks ...
Syncing rietveld...
Running push-basic.sh
Running save-description-on-failure.sh
Running basic.sh
Running upload-stale.sh
Running upload-local-tracking-branch.sh
Running patch.sh
Running submit-from-new-dir.sh
Running post-dcommit-hook-test.sh
Running hooks.sh
Running abandon.sh
Running tbr.sh
Running upstream.sh

** Presubmit Messages **
You should install python 2.5 and run ln -s $(which python2.5) python.
A great place to put this symlink is in depot_tools.
Otherwise, you break depot_tools on python 2.5, you get to keep the pieces.

** Presubmit ERRORS **
tests/gclient_smoketest.py failed!
Command tests/gclient_smoketest.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/depot_tools
.......F...............................
======================================================================
FAIL: testMultiSolutionsJobs (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1009, in testMultiSolutionsJobs
    untangle=True)
  File "tests/gclient_smoketest.py", line 77, in parseGclient
    return self.checkBlock(stdout, items)
  File "tests/gclient_smoketest.py", line 137, in checkBlock
    self.checkString(results[i][0][2], path, (i, results[i][0][2], path))
  File "/mnt/data/b/commit-queue/workdir/depot_tools/tests/fake_repos.py", line
672, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: (3,
'/tmp/trial5R7Ny1/__main__.GClientSmokeBoth.testMultiSolutionsJobs',
'/tmp/trial5R7Ny1/__main__.GClientSmokeBoth.testMultiSolutionsJobs/src/file/other')

----------------------------------------------------------------------
Ran 39 tests in 191.908s

FAILED (failures=1)


tests/gclient_scm_test.py failed!
Command tests/gclient_scm_test.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/depot_tools
...........EE...................
======================================================================
ERROR: testUpdateUnstagedConflict (__main__.GitWrapperTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_scm_test.py", line 757, in testUpdateUnstagedConflict
    scm.update(options, (), [])
  File "/mnt/data/b/commit-queue/workdir/depot_tools/gclient_scm.py", line 395,
in update
    raise gclient_utils.Error(e.stderr)
Error: error: Your local changes to 'b' would be overwritten by merge. 
Aborting.
Please, commit your changes or stash them before you can merge.


======================================================================
ERROR: testUpdateUnstagedConflict (__main__.GitWrapperTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_scm_test.py", line 539, in tearDown
    StdoutCheck.tearDown(self)
  File "/mnt/data/b/commit-queue/workdir/depot_tools/tests/super_mox.py", line
100, in tearDown
    self.assertEquals('', sys.stdout.getvalue())
AssertionError: '' != '\n_____ . at refs/heads/master\n'

----------------------------------------------------------------------
Ran 31 tests in 1.278s

FAILED (errors=2)


tests/presubmit_unittest.py failed!
Command tests/presubmit_unittest.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/depot_tools
..............................................................F............................
======================================================================
FAIL: testGetAbsoluteLocalPath (__main__.InputApiUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mnt/data/b/commit-queue/workdir/depot_tools/third_party/pymox/mox.py",
line 1618, in new_method
    func(self, *args, **kwargs)
  File "tests/presubmit_unittest.py", line 1062, in testGetAbsoluteLocalPath
    paths_from_api = api.AbsoluteLocalPaths(include_dirs=True)
  File "/mnt/data/b/commit-queue/workdir/depot_tools/presubmit_support.py", line
341, in AbsoluteLocalPaths
    return [af.AbsoluteLocalPath() for af in self.AffectedFiles(include_dirs)]
  File "/mnt/data/b/commit-queue/workdir/depot_tools/presubmit_support.py", line
333, in AffectedFiles
    self.change.AffectedFiles(include_dirs, include_deletes))
  File "/mnt/data/b/commit-queue/workdir/depot_tools/presubmit_support.py", line
737, in AffectedFiles
    affected = filter(lambda x: not x.IsDirectory(), self._affected_files)
  File "/mnt/data/b/commit-queue/workdir/depot_tools/presubmit_support.py", line
737, in <lambda>
    affected = filter(lambda x: not x.IsDirectory(), self._affected_files)
  File "/mnt/data/b/commit-queue/workdir/depot_tools/presubmit_support.py", line
457, in IsDirectory
    self._is_directory = (os.path.exists(path) and
  File "/mnt/data/b/commit-queue/workdir/depot_tools/third_party/pymox/mox.py",
line 771, in __call__
    expected_method = self._VerifyMethodCall()
  File "/mnt/data/b/commit-queue/workdir/depot_tools/third_party/pymox/mox.py",
line 816, in _VerifyMethodCall
    expected = self._PopNextMethod()
  File "/mnt/data/b/commit-queue/workdir/depot_tools/third_party/pymox/mox.py",
line 802, in _PopNextMethod
    raise UnexpectedMethodCallError(self, None)
UnexpectedMethodCallError: Unexpected method call Stub for <function exists at
0x7f8b5552af50>.__call__('/RZn/sLc/iaN/aFQ/isdir') -> None

----------------------------------------------------------------------
Ran 91 tests in 0.259s

FAILED (failures=1)


upstream.sh failed
Command /mnt/data/b/commit-queue/workdir/depot_tools/tests/upstream.sh returned
non-zero exit status 1 in /mnt/data/b/commit-queue/workdir/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
error: unknown switch `B'
usage: git checkout [options] <branch>
   or: git checkout [options] [<branch>] -- <file>...

    -q, --quiet           be quiet
    -b <new branch>       branch
    -l                    log for new branch
    -t, --track           track
    -2, --ours            stage
    -3, --theirs          stage
    -f, --force           force
    -m, --merge           merge
    --conflict <style>    conflict style (merge or diff3)
    -p, --patch           select hunks interactively



Presubmit checks took 365.9s to calculate.

[M-A Ruel, 2011-05-06 13:30:50]

One error is genuine, I'll take a look.

[M-A Ruel, 2011-05-06 13:54:13]

On 2011/05/06 13:30:50, Marc-Antoine Ruel wrote:
> One error is genuine, I'll take a look.

I disagree with the patch as there are callers that supplies the include_dirs
argument as a positional argument. It makes fixing the code way more complex
than necessary and breaks the convention about AffectedFiles(), the position of
arguments differs from Change.AffectedFiles(). Please add at the end and push
the file_filter into Change.AffectedFiles() so users aren't surprised by
input_api.change.AffectedFiles() not working as expected.

[sail, 2011-05-09 19:51:05]

Fixed presubmit errors and addressed review comments. Please take another look.
Thanks!

[M-A Ruel, 2011-05-09 20:13:44]

It doesn't pass clean here:


(test2=b1ce77) ~/src/depot_tools> git cl patch 6932060
Current branch test2 is up to date.
Loaded authentication cookies from /home/maruel/.codereview_upload_cookies
Committed patch.
(test2=1f6424) ~/src/depot_tools> git cl presubmit
Current branch test2 is up to date.
Loaded authentication cookies from /home/maruel/.codereview_upload_cookies
Running presubmit commit checks ...
************* Module presubmit_support
W0108:738:Change.AffectedFiles.<lambda>: Lambda may not be necessary
************* Module tests.presubmit_unittest
E0001:1395: non-keyword arg after keyword arg
Syncing rietveld...
Running patch.sh
Running upload-local-tracking-branch.sh
Running tbr.sh
Running abandon.sh
Running basic.sh
Running hooks.sh
Running push-basic.sh
Running post-dcommit-hook-test.sh
Running submit-from-new-dir.sh
Running upstream.sh
Running upload-stale.sh
Running save-description-on-failure.sh

** Presubmit ERRORS **
Fix pylint errors first.

tests/presubmit_unittest.py failed!
Command tests/presubmit_unittest.py returned non-zero exit status 1 in
/mnt/seagate1.5tb/maruel/src/depot_tools
  File "tests/presubmit_unittest.py", line 1395
    input_api1.AffectedFiles(mox.IgnoreArg(), include_deletes=False,
None).AndReturn(
SyntaxError: non-keyword arg after keyword arg


Presubmit checks took 167.8s to calculate.

http://codereview.chromium.org/6932060/diff/10001/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6932060/diff/10001/presubmit_support.py#newcod...
presubmit_support.py:738: affected = filter(lambda x: file_filter(x), affected)
You can simplify that:

affected = filter(file_filter, affected)

because:
- "lambda x: file_filter(x)" == "file_filter"
- filter(None, affected) works. Look up the docs for more info.

so no need for "if file_filter". Awesome, eh?

http://codereview.chromium.org/6932060/diff/10001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6932060/diff/10001/tests/presubmit_unittest.py...
tests/presubmit_unittest.py:1395: input_api1.AffectedFiles(mox.IgnoreArg(),
include_deletes=False, None).AndReturn(
80 cols.

[sail, 2011-05-09 21:01:33]

On 2011/05/09 20:13:44, Marc-Antoine Ruel wrote:
> It doesn't pass clean here:

I'm getting errors like this:
UnexpectedMethodCallError: Unexpected method call.  unexpected:-  expected:+
- AffectedFiles(file_filter=<function filter_more at 0x2bc3230>,
include_deletes=False) -> None
+ AffectedFiles(file_filter=<IgnoreArg>, include_deletes=False,
include_dirs=<IgnoreArg>) -> [<MockAnything instance>]

Do you know how I can fix this? Here's the code that I think is generating this
error:
    input_api1.AffectedFiles(include_dirs=mox.IgnoreArg(),
include_deletes=False,
                            
file_filter=mox.IgnoreArg()).AndReturn([affected_file])

Thanks

[M-A Ruel, 2011-05-10 13:40:31]

On 2011/05/09 21:01:33, sail wrote:
> On 2011/05/09 20:13:44, Marc-Antoine Ruel wrote:
> > It doesn't pass clean here:
> 
> I'm getting errors like this:
> UnexpectedMethodCallError: Unexpected method call.  unexpected:-  expected:+
> - AffectedFiles(file_filter=<function filter_more at 0x2bc3230>,
> include_deletes=False) -> None

The actual call included file_filter and include_deletes arguments.

> + AffectedFiles(file_filter=<IgnoreArg>, include_deletes=False,
> include_dirs=<IgnoreArg>) -> [<MockAnything instance>]

But you also expected include_dirs argument.

> Do you know how I can fix this? Here's the code that I think is generating
this
> error:
>     input_api1.AffectedFiles(include_dirs=mox.IgnoreArg(),
> include_deletes=False,
>                             
> file_filter=mox.IgnoreArg()).AndReturn([affected_file])

So use instead:

input_api1.AffectedFiles(
  include_deletes=False, file_filter=mox.IgnoreArg()).AndReturn([affected_file])

[sail, 2011-05-10 16:53:25]

Fixed presubmit test. Thanks!

http://codereview.chromium.org/6932060/diff/10001/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6932060/diff/10001/presubmit_support.py#newcod...
presubmit_support.py:738: affected = filter(lambda x: file_filter(x), affected)
On 2011/05/09 20:13:44, Marc-Antoine Ruel wrote:
> You can simplify that:
> 
> affected = filter(file_filter, affected)
> 
> because:
> - "lambda x: file_filter(x)" == "file_filter"
> - filter(None, affected) works. Look up the docs for more info.
> 
> so no need for "if file_filter". Awesome, eh?

Done. Cool!

http://codereview.chromium.org/6932060/diff/10001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6932060/diff/10001/tests/presubmit_unittest.py...
tests/presubmit_unittest.py:1395: input_api1.AffectedFiles(mox.IgnoreArg(),
include_deletes=False, None).AndReturn(
On 2011/05/09 20:13:44, Marc-Antoine Ruel wrote:
> 80 cols.

Done.

[M-A Ruel, 2011-05-10 17:18:48]

Fix these and lgtm.

http://codereview.chromium.org/6932060/diff/15001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6932060/diff/15001/tests/presubmit_unittest.py...
tests/presubmit_unittest.py:1395:
input_api1.AffectedFiles(include_deletes=False,
************* Module tests.presubmit_unittest
C0301:1396: Line too long (84/80)
C0301:1409: Line too long (84/80)

[sail, 2011-05-10 17:25:12]

http://codereview.chromium.org/6932060/diff/15001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6932060/diff/15001/tests/presubmit_unittest.py...
tests/presubmit_unittest.py:1395:
input_api1.AffectedFiles(include_deletes=False,
On 2011/05/10 17:18:48, Marc-Antoine Ruel wrote:
> ************* Module tests.presubmit_unittest
> C0301:1396: Line too long (84/80)
> C0301:1409: Line too long (84/80)

Done.

[commit-bot: I haz the power, 2011-05-12 15:06:12]

Presubmit check for 6932060-18001 failed and returned exit status 1.

Running presubmit commit checks ...
Syncing rietveld...
Running push-basic.sh
Running save-description-on-failure.sh
Running basic.sh
Running upload-stale.sh
Running upload-local-tracking-branch.sh
Running patch.sh
Running submit-from-new-dir.sh
Running post-dcommit-hook-test.sh
Running hooks.sh
Running abandon.sh
Running tbr.sh
Running upstream.sh

** Presubmit Messages **
You should install python 2.5 and run ln -s $(which python2.5) python.
A great place to put this symlink is in depot_tools.
Otherwise, you break depot_tools on python 2.5, you get to keep the pieces.

** Presubmit ERRORS **
tests/gclient_smoketest.py failed!
Command tests/gclient_smoketest.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/depot_tools
.......F...............................
======================================================================
FAIL: testMultiSolutionsJobs (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1009, in testMultiSolutionsJobs
    untangle=True)
  File "tests/gclient_smoketest.py", line 77, in parseGclient
    return self.checkBlock(stdout, items)
  File "tests/gclient_smoketest.py", line 137, in checkBlock
    self.checkString(results[i][0][2], path, (i, results[i][0][2], path))
  File "/mnt/data/b/commit-queue/workdir/depot_tools/tests/fake_repos.py", line
672, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: (3,
'/tmp/trialrOBfqj/__main__.GClientSmokeBoth.testMultiSolutionsJobs',
'/tmp/trialrOBfqj/__main__.GClientSmokeBoth.testMultiSolutionsJobs/src/file/other')

----------------------------------------------------------------------
Ran 39 tests in 193.958s

FAILED (failures=1)


tests/gclient_scm_test.py failed!
Command tests/gclient_scm_test.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/depot_tools
...........EE...................
======================================================================
ERROR: testUpdateUnstagedConflict (__main__.GitWrapperTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_scm_test.py", line 757, in testUpdateUnstagedConflict
    scm.update(options, (), [])
  File "/mnt/data/b/commit-queue/workdir/depot_tools/gclient_scm.py", line 395,
in update
    raise gclient_utils.Error(e.stderr)
Error: error: Your local changes to 'b' would be overwritten by merge. 
Aborting.
Please, commit your changes or stash them before you can merge.


======================================================================
ERROR: testUpdateUnstagedConflict (__main__.GitWrapperTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_scm_test.py", line 539, in tearDown
    StdoutCheck.tearDown(self)
  File "/mnt/data/b/commit-queue/workdir/depot_tools/tests/super_mox.py", line
100, in tearDown
    self.assertEquals('', sys.stdout.getvalue())
AssertionError: '' != '\n_____ . at refs/heads/master\n'

----------------------------------------------------------------------
Ran 31 tests in 1.116s

FAILED (errors=2)


upstream.sh failed
Command /mnt/data/b/commit-queue/workdir/depot_tools/tests/upstream.sh returned
non-zero exit status 1 in /mnt/data/b/commit-queue/workdir/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
error: unknown switch `B'
usage: git checkout [options] <branch>
   or: git checkout [options] [<branch>] -- <file>...

    -q, --quiet           be quiet
    -b <new branch>       branch
    -l                    log for new branch
    -t, --track           track
    -2, --ours            stage
    -3, --theirs          stage
    -f, --force           force
    -m, --merge           merge
    --conflict <style>    conflict style (merge or diff3)
    -p, --patch           select hunks interactively



Presubmit checks took 357.0s to calculate.