Skip to content

Commit 5c8c2ba

Browse files
authored
[CLI] Migrate lint (#5045)
### Changes This PR introduces a new `lint` command to the `casp` CLI, which acts as a convenient wrapper for the root `butler.py lint` script. To enable `casp` to target specific files or directories for linting, the `butler.py lint` command was updated to accept a `--path` argument. The `build_command` function in `casp.utils.local_butler` now checks if the value in the arguments list is `None`. If so, it adds only the argument to the command. The `format` command was updated to match the `lint` implementation. A specific test was added for the case of receiving flags in the util module. This behavior was also added to the similar function in `casp.utils.container`. This is needed to support flag arguments, like `--type-check` in the lint command. ### How it works: - `casp lint` without arguments will lint all changed files in the current branch, preserving the default `butler` behavior. - `casp lint <path>` or `casp lint --path <path>` will now run the linter exclusively on the specified file or directory. - `casp lint <path> --type-check` will also run the type checking ### Changes in butler Regarding the refactor/improvement in `butler.py`, here are some screenshots comparing running `python butler.py lint` in this branch and master. They show that the behavior is the same. <img width="1285" height="161" alt="image" src="https://github.com/user-attachments/assets/dfb35448-757f-4ad9-99bc-c1d24ae39ab0" /> <img width="1285" height="161" alt="image" src="https://github.com/user-attachments/assets/09a8fb0b-17eb-4978-b3bc-14f69ab97fcb" /> ### Demo prints Here are some screenshots of `casp lint` in action: 1. Finding a lint error: <img width="1783" height="815" alt="image" src="https://github.com/user-attachments/assets/bee53f78-98bf-4dc6-8fb7-612741a5fa58" /> 2. Fixing with `casp format` (passing the exact file which has the error): <img width="1783" height="166" alt="image" src="https://github.com/user-attachments/assets/1ac5df97-e080-479c-b98d-5681d1edc73a" /> 3. Linting again, now passed. <img width="1784" height="351" alt="image" src="https://github.com/user-attachments/assets/437f6184-b401-4cfc-be25-5f190b1bfa0b" />
1 parent 3612e16 commit 5c8c2ba

File tree

8 files changed

+219
-23
lines changed

8 files changed

+219
-23
lines changed

butler.py

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,23 @@ def _setup_args_for_remote(parser):
9898
subparsers.add_parser('reboot', help='Reboot with `sudo reboot`.')
9999

100100

101+
def _add_lint_subparser(toplevel_subparsers):
102+
"""Adds a parser for the `lint` command."""
103+
parser = toplevel_subparsers.add_parser(
104+
'lint', help='Lint changed code in current branch.')
105+
parser.add_argument(
106+
'--type-check',
107+
help='Also run the type checker on changed files.',
108+
action='store_true',
109+
default=False)
110+
parser.add_argument(
111+
'--path',
112+
dest='path',
113+
default=None,
114+
help=('The file or directory to lint. Default is to lint changed '
115+
'files in current branch'))
116+
117+
101118
def _add_format_subparser(toplevel_subparsers):
102119
"""Adds a parser for the `format` command."""
103120
parser = toplevel_subparsers.add_parser(
@@ -308,14 +325,6 @@ def main():
308325
help=('Do not close browser when tests '
309326
'finish. Good for debugging.'))
310327

311-
parser_lint = subparsers.add_parser(
312-
'lint', help='Lint changed code in current branch.')
313-
parser_lint.add_argument(
314-
'--type-check',
315-
help='Also run the type checker on changed files.',
316-
action='store_true',
317-
default=False)
318-
319328
parser_package = subparsers.add_parser(
320329
'package', help='Package clusterfuzz with a staging revision')
321330
parser_package.add_argument(
@@ -439,6 +448,7 @@ def main():
439448
default='us-central',
440449
help='Location for App Engine.')
441450

451+
_add_lint_subparser(subparsers)
442452
_add_format_subparser(subparsers)
443453
_add_integration_tests_subparsers(subparsers)
444454
_add_weights_subparser(subparsers)

cli/casp/src/casp/commands/format.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ def cli(path: str, path_option: str) -> None:
3535
target_dir = path_option or path
3636

3737
try:
38+
arguments = {}
3839
if target_dir:
39-
command = local_butler.build_command('format', path=target_dir)
40-
else:
41-
command = local_butler.build_command('format')
40+
arguments['path'] = target_dir
41+
command = local_butler.build_command('format', **arguments)
4242
except FileNotFoundError:
4343
click.echo('butler.py not found in this directory.', err=True)
4444
sys.exit(1)

cli/casp/src/casp/commands/lint.py

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,57 @@
1111
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
14-
"""Lint command."""
14+
"""Run lint command."""
1515

16+
import subprocess
17+
import sys
18+
19+
from casp.utils import local_butler
1620
import click
1721

1822

19-
@click.command(name='lint', help='Lint changed code in current branch.')
20-
def cli():
21-
"""Lint changed code in current branch."""
22-
click.echo('To be implemented...')
23+
@click.command(
24+
name='lint',
25+
help=('Runs linting checks on the codebase. '
26+
'By default, it lints changed code in the current branch. '
27+
'You can also specify a file or directory to lint.'))
28+
@click.argument('path', required=False, type=click.Path(exists=True))
29+
@click.option(
30+
'--path',
31+
'-p',
32+
'path_option',
33+
help='The file or directory to run the lint command in.',
34+
default=None,
35+
type=click.Path(exists=True),
36+
show_default=True)
37+
@click.option(
38+
'--type-check',
39+
help='Also run the type checker',
40+
is_flag=True,
41+
default=False)
42+
def cli(path: str, path_option: str, type_check: bool) -> None:
43+
"""Runs linting checks on the codebase.
44+
45+
By default, it lints changed code in the current branch.
46+
You can also specify a file or directory to lint."""
47+
target_dir = path_option or path
48+
49+
try:
50+
arguments = {}
51+
if type_check:
52+
arguments['type_check'] = None
53+
if target_dir:
54+
arguments['path'] = target_dir
55+
command = local_butler.build_command('lint', **arguments)
56+
except FileNotFoundError:
57+
click.echo('butler.py not found in this directory.', err=True)
58+
sys.exit(1)
59+
60+
try:
61+
subprocess.run(command, check=True)
62+
except FileNotFoundError:
63+
click.echo('python not found in PATH.', err=True)
64+
sys.exit(1)
65+
except subprocess.CalledProcessError as e:
66+
click.echo(f'Error running butler.py lint: {e}', err=True)
67+
sys.exit(1)
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
# Copyright 2025 Google LLC
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
"""Tests for the lint command.
15+
16+
For running all the tests, use (from the root of the project):
17+
python -m unittest discover -s cli/casp/src/casp/tests -p test_lint.py -v
18+
"""
19+
20+
from pathlib import Path
21+
import subprocess
22+
import unittest
23+
from unittest.mock import patch
24+
25+
from casp.commands import lint as lint_command
26+
from click.testing import CliRunner
27+
28+
29+
class LintCliTest(unittest.TestCase):
30+
"""Tests for the lint command."""
31+
32+
def setUp(self):
33+
self.runner = CliRunner()
34+
self.mock_local_butler = self.enterContext(
35+
patch('casp.commands.lint.local_butler', autospec=True))
36+
self.mock_subprocess_run = self.enterContext(
37+
patch('subprocess.run', autospec=True))
38+
39+
def test_lint_success_no_args(self):
40+
"""Tests successful execution of `casp lint` without arguments."""
41+
self.mock_local_butler.build_command.return_value = ['cmd']
42+
result = self.runner.invoke(lint_command.cli)
43+
self.assertEqual(0, result.exit_code, msg=result.output)
44+
self.mock_local_butler.build_command.assert_called_once_with('lint')
45+
self.mock_subprocess_run.assert_called_once_with(['cmd'], check=True)
46+
47+
def test_lint_success_with_path_arg(self):
48+
"""Tests `casp lint <some_dir>` (positional argument)."""
49+
self.mock_local_butler.build_command.return_value = ['cmd']
50+
with self.runner.isolated_filesystem():
51+
Path('test_dir').mkdir()
52+
result = self.runner.invoke(lint_command.cli, ['test_dir'])
53+
self.assertEqual(0, result.exit_code, msg=result.output)
54+
self.mock_local_butler.build_command.assert_called_once_with(
55+
'lint', path='test_dir')
56+
self.mock_subprocess_run.assert_called_once_with(['cmd'], check=True)
57+
58+
def test_lint_success_with_path_option(self):
59+
"""Tests `casp lint --path <some_dir>`."""
60+
self.mock_local_butler.build_command.return_value = ['cmd']
61+
with self.runner.isolated_filesystem():
62+
Path('test_dir').mkdir()
63+
result = self.runner.invoke(lint_command.cli, ['--path', 'test_dir'])
64+
self.assertEqual(0, result.exit_code, msg=result.output)
65+
self.mock_local_butler.build_command.assert_called_once_with(
66+
'lint', path='test_dir')
67+
self.mock_subprocess_run.assert_called_once_with(['cmd'], check=True)
68+
69+
def test_lint_success_with_type_check(self):
70+
"""Tests `casp lint --type-check`."""
71+
self.mock_local_butler.build_command.return_value = ['cmd']
72+
result = self.runner.invoke(lint_command.cli, ['--type-check'])
73+
self.assertEqual(0, result.exit_code, msg=result.output)
74+
self.mock_local_butler.build_command.assert_called_once_with(
75+
'lint', type_check=None)
76+
self.mock_subprocess_run.assert_called_once_with(['cmd'], check=True)
77+
78+
def test_lint_success_with_path_and_type_check(self):
79+
"""Tests `casp lint <some_dir> --type-check`."""
80+
self.mock_local_butler.build_command.return_value = ['cmd']
81+
with self.runner.isolated_filesystem():
82+
Path('test_dir').mkdir()
83+
result = self.runner.invoke(lint_command.cli,
84+
['test_dir', '--type-check'])
85+
self.assertEqual(0, result.exit_code, msg=result.output)
86+
self.mock_local_butler.build_command.assert_called_once_with(
87+
'lint', path='test_dir', type_check=None)
88+
self.mock_subprocess_run.assert_called_once_with(['cmd'], check=True)
89+
90+
def test_butler_not_found(self):
91+
"""Tests when `butler.py` is not found."""
92+
self.mock_local_butler.build_command.side_effect = FileNotFoundError
93+
result = self.runner.invoke(lint_command.cli)
94+
self.assertNotEqual(0, result.exit_code)
95+
self.assertIn('butler.py not found', result.output)
96+
self.mock_subprocess_run.assert_not_called()
97+
98+
def test_subprocess_run_fails(self):
99+
"""Tests when `subprocess.run` fails."""
100+
self.mock_local_butler.build_command.return_value = ['cmd']
101+
self.mock_subprocess_run.side_effect = subprocess.CalledProcessError(
102+
1, 'cmd')
103+
result = self.runner.invoke(lint_command.cli)
104+
self.assertNotEqual(0, result.exit_code)
105+
self.assertIn('Error running butler.py lint', result.output)
106+
107+
def test_python_not_found(self):
108+
"""Tests when `python` command is not found."""
109+
self.mock_local_butler.build_command.return_value = ['cmd']
110+
self.mock_subprocess_run.side_effect = FileNotFoundError
111+
result = self.runner.invoke(lint_command.cli)
112+
self.assertNotEqual(0, result.exit_code)
113+
self.assertIn('python not found in PATH', result.output)
114+
115+
116+
if __name__ == '__main__':
117+
unittest.main()

cli/casp/src/casp/tests/utils/test_local_butler.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,24 @@ def test_build_command_success_explicit_path(self):
4949
self.assertEqual(
5050
command, ['python', '/explicit/butler.py', 'format', '--verbose=true'])
5151

52+
@patch('casp.utils.path_utils.get_butler_in_dir', autospec=True)
53+
def test_build_command_flags_and_args(self, mock_get_butler):
54+
"""Tests handling of flags (None value) and args with underscores."""
55+
mock_get_butler.return_value = Path('/fake/butler.py')
56+
57+
command = local_butler.build_command(
58+
'lint',
59+
path='some/path',
60+
type_check=None,
61+
another_flag=None,
62+
complex_arg='value')
63+
64+
expected_command = [
65+
'python', '/fake/butler.py', 'lint', '--path=some/path', '--type-check',
66+
'--another-flag', '--complex-arg=value'
67+
]
68+
self.assertEqual(command, expected_command)
69+
5270
@patch('casp.utils.path_utils.get_butler_in_dir', autospec=True)
5371
def test_butler_not_found(self, mock_get_butler):
5472
"""Tests validation when butler.py is not found."""

cli/casp/src/casp/utils/container.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ def build_butler_command(subcommand: str, **kwargs: str) -> list[str]:
4747
command = f'{_COMMAND_PREFIX} {subcommand}'
4848
for key, value in kwargs.items():
4949
key = key.replace('_', '-')
50-
command += f' --{key}={value}'
50+
if value is not None:
51+
command += f' --{key}={value}'
52+
else:
53+
command += f' --{key}'
5154

5255
return ['bash', '-c', command]

cli/casp/src/casp/utils/local_butler.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,13 @@
1414
"""Local butler command utilities."""
1515

1616
from pathlib import Path
17-
from typing import Optional
1817

1918
from casp.utils import path_utils
2019

2120

2221
def build_command(subcommand: str,
23-
butler_path: Optional[Path] = None,
24-
**kwargs: str) -> list[str]:
22+
butler_path: Path | None = None,
23+
**kwargs: str | None) -> list[str]:
2524
"""Builds a butler command for local execution.
2625
2726
Args:
@@ -47,6 +46,9 @@ def build_command(subcommand: str,
4746
command = ['python', str(butler_path), subcommand]
4847
for key, value in kwargs.items():
4948
key = key.replace('_', '-')
50-
command.append(f'--{key}={value}')
49+
if value is not None:
50+
command.append(f'--{key}={value}')
51+
else:
52+
command.append(f'--{key}')
5153

5254
return command

src/local/butler/lint.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,9 @@ def execute(args):
175175
third_party_path = os.path.join(module_parent_path, 'third_party')
176176
os.environ['PYTHONPATH'] = ':'.join(
177177
[third_party_path, module_parent_path, pythonpath])
178-
179-
if 'GOOGLE_CLOUDBUILD' in os.environ:
178+
if args.path:
179+
diff_command = f'git ls-files {os.path.abspath(args.path)}'
180+
elif 'GOOGLE_CLOUDBUILD' in os.environ:
180181
# Explicitly compare against master if we're running on the CI
181182
diff_command = 'git diff --name-only master FETCH_HEAD'
182183
else:

0 commit comments

Comments
 (0)