From 075cd76fe6433ac4ec218e797263ad26cfd02c66 Mon Sep 17 00:00:00 2001 From: Aravind Vasudevan Date: Wed, 23 Mar 2022 21:13:13 +0000 Subject: [PATCH] Update argument parsing to use argparse This CL updates argument parsing mechanism from using a custom implementation + optparse to argparse. It is intended to clear tech debt for future changes. Change-Id: I06ba5d3c532638a7970be960e02ebbafbea9dcb0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3541479 Reviewed-by: Gavin Mak Reviewed-by: Josip Sokcevic Commit-Queue: Aravind Vasudevan --- fetch.py | 117 +++++++++++++++----------------------------- tests/fetch_test.py | 97 ++++++++---------------------------- 2 files changed, 60 insertions(+), 154 deletions(-) diff --git a/fetch.py b/fetch.py index 2b7245df4f..f19f103bd7 100755 --- a/fetch.py +++ b/fetch.py @@ -21,7 +21,7 @@ These parameters will be passed through to the config's main method. from __future__ import print_function import json -import optparse +import argparse import os import pipes import subprocess @@ -174,85 +174,48 @@ def CheckoutFactory(type_name, options, spec, root): raise KeyError('unrecognized checkout type: %s' % type_name) return class_(options, spec, root) - -################################################# -# Utility function and file entry point. -################################################# -def usage(msg=None): - """Print help and exit.""" - if msg: - print('Error:', msg) - - print(textwrap.dedent("""\ - usage: %s [options] [--property=value [--property2=value2 ...]] - - This script can be used to download the Chromium sources. See - http://www.chromium.org/developers/how-tos/get-the-code - for full usage instructions. - - Valid options: - -h, --help, help Print this message. - --nohooks Don't run hooks after checkout. - --force (dangerous) Don't look for existing .gclient file. - -n, --dry-run Don't run commands, only print them. - --no-history Perform shallow clones, don't fetch the full git history. - - Valid fetch configs:""") % os.path.basename(sys.argv[0])) +def handle_args(argv): + """Gets the config name from the command line arguments.""" configs_dir = os.path.join(SCRIPT_PATH, 'fetch_configs') configs = [f[:-3] for f in os.listdir(configs_dir) if f.endswith('.py')] configs.sort() - for fname in configs: - print(' ' + fname) - sys.exit(bool(msg)) - - -def handle_args(argv): - """Gets the config name from the command line arguments.""" - if len(argv) <= 1: - usage('Must specify a config.') - if argv[1] in ('-h', '--help', 'help'): - usage() - - dry_run = False - nohooks = False - no_history = False - force = False - while len(argv) >= 2: - arg = argv[1] - if not arg.startswith('-'): - break - argv.pop(1) - if arg in ('-n', '--dry-run'): - dry_run = True - elif arg == '--nohooks': - nohooks = True - elif arg == '--no-history': - no_history = True - elif arg == '--force': - force = True - else: - usage('Invalid option %s.' % arg) - - def looks_like_arg(arg): - return arg.startswith('--') and arg.count('=') == 1 - - bad_parms = [x for x in argv[2:] if not looks_like_arg(x)] - if bad_parms: - usage('Got bad arguments %s' % bad_parms) - - config = argv[1] - props = argv[2:] - return ( - optparse.Values({ - 'dry_run': dry_run, - 'nohooks': nohooks, - 'no_history': no_history, - 'force': force}), - config, - props) + parser = argparse.ArgumentParser( + formatter_class=argparse.RawDescriptionHelpFormatter, + description=''' + This script can be used to download the Chromium sources. See + http://www.chromium.org/developers/how-tos/get-the-code + for full usage instructions.''', + epilog='Valid fetch configs:\n' + \ + '\n'.join(map(lambda s: ' ' + s, configs)) + ) + + parser.add_argument('-n', '--dry-run', action='store_true', default=False, + help='Don\'t run commands, only print them.') + parser.add_argument('--nohooks', action='store_true', default=False, + help='Don\'t run hooks after checkout.') + parser.add_argument('--no-history', action='store_true', default=False, + help='Perform shallow clones, don\'t fetch the full git history.') + parser.add_argument('--force', action='store_true', default=False, + help='(dangerous) Don\'t look for existing .gclient file.') + + parser.add_argument('config', type=str, + help="Project to fetch, e.g. chromium.") + parser.add_argument('props', metavar='props', type=str, + nargs=argparse.REMAINDER, default=[]) + + args = parser.parse_args(argv[1:]) + + # props passed to config must be of the format --= + looks_like_arg = lambda arg: arg.startswith('--') and arg.count('=') == 1 + bad_param = [x for x in args.props if not looks_like_arg(x)] + if bad_param: + print('Error: Got bad arguments %s' % bad_param) + parser.print_help() + sys.exit(1) + return args def run_config_fetch(config, props, aliased=False): """Invoke a config's fetch method with the passed-through args @@ -305,9 +268,9 @@ def run(options, spec, root): def main(): - options, config, props = handle_args(sys.argv) - spec, root = run_config_fetch(config, props) - return run(options, spec, root) + args = handle_args(sys.argv) + spec, root = run_config_fetch(args.config, args.props) + return run(args, spec, root) if __name__ == '__main__': diff --git a/tests/fetch_test.py b/tests/fetch_test.py index 5552ece65c..74ba06cd40 100755 --- a/tests/fetch_test.py +++ b/tests/fetch_test.py @@ -7,7 +7,7 @@ import contextlib import logging -import optparse +import argparse import os import subprocess import sys @@ -31,39 +31,9 @@ import fetch class SystemExitMock(Exception): pass - -class UsageMock(Exception): - pass - - class TestUtilityFunctions(unittest.TestCase): """This test case is against utility functions""" - @mock.patch('sys.stdout', StringIO()) - @mock.patch('os.listdir', return_value=['foo.py', 'bar']) - @mock.patch('sys.exit', side_effect=SystemExitMock) - def test_usage_with_message(self, exit_mock, listdir): - with self.assertRaises(SystemExitMock): - fetch.usage('foo') - exit_mock.assert_called_once_with(1) - listdir.assert_called_once() - - stdout = sys.stdout.getvalue() - self.assertTrue(stdout.startswith('Error: foo')) - - self._usage_static_message(stdout) - - @mock.patch('sys.stdout', StringIO()) - @mock.patch('os.listdir', return_value=['foo.py', 'bar']) - @mock.patch('sys.exit', side_effect=SystemExitMock) - def test_usage_with_no_message(self, exit_mock, listdir): - with self.assertRaises(SystemExitMock): - fetch.usage() - exit_mock.assert_called_once_with(0) - listdir.assert_called_once() - - self._usage_static_message(sys.stdout.getvalue()) - def _usage_static_message(self, stdout): valid_fetch_config_text = 'Valid fetch configs:' self.assertIn(valid_fetch_config_text, stdout) @@ -76,51 +46,27 @@ class TestUtilityFunctions(unittest.TestCase): self.assertIn('foo', split[1]) self.assertNotIn('bar', split[1]) - @mock.patch('fetch.usage', side_effect=UsageMock) - def test_handle_args_invalid_usage(self, usage): - with self.assertRaises(UsageMock): - fetch.handle_args(['filename', '-h']) - usage.assert_called_with() - - with self.assertRaises(UsageMock): - fetch.handle_args(['filename']) - usage.assert_called_with('Must specify a config.') - - with self.assertRaises(UsageMock): - fetch.handle_args(['filename', '--foo']) - usage.assert_called_with('Invalid option --foo.') - - bad_arguments = [ - '--not-valid-param', '-not-valid-param=1', '--not-valid-param=1=2' - ] - - for bad_argument in bad_arguments: - with self.assertRaises(UsageMock): - fetch.handle_args(['filename', 'foo', bad_argument]) - usage.assert_called_with('Got bad arguments [\'%s\']' % (bad_argument)) - - @mock.patch('optparse.Values', return_value=None) - def test_handle_args_valid_usage(self, values): + def test_handle_args_valid_usage(self): response = fetch.handle_args(['filename', 'foo']) - values.assert_called_with({ - 'dry_run': False, - 'nohooks': False, - 'no_history': False, - 'force': False - }) - self.assertEqual((None, 'foo', []), response) + self.assertEqual(argparse.Namespace( + dry_run=False, + nohooks=False, + no_history=False, + force=False, + config='foo', + props=[]), response) response = fetch.handle_args([ 'filename', '-n', '--dry-run', '--nohooks', '--no-history', '--force', 'foo', '--some-param=1', '--bar=2' ]) - values.assert_called_with({ - 'dry_run': True, - 'nohooks': True, - 'no_history': True, - 'force': True - }) - self.assertEqual((None, 'foo', ['--some-param=1', '--bar=2']), response) + self.assertEqual(argparse.Namespace( + dry_run=True, + nohooks=True, + no_history=True, + force=True, + config='foo', + props=['--some-param=1', '--bar=2']), response) @mock.patch('os.path.exists', return_value=False) @mock.patch('sys.stdout', StringIO()) @@ -166,7 +112,7 @@ class TestCheckout(unittest.TestCase): mock.patch('sys.stdout', StringIO()).start() self.addCleanup(mock.patch.stopall) - self.opts = optparse.Values({'dry_run': False}) + self.opts = argparse.Namespace(dry_run=False) self.checkout = fetch.Checkout(self.opts, {}, '') @contextlib.contextmanager @@ -246,7 +192,7 @@ class TestGClientCheckout(unittest.TestCase): def setUp(self): self.run = mock.patch('fetch.Checkout.run').start() - self.opts = optparse.Values({'dry_run': False}) + self.opts = argparse.Namespace(dry_run=False) self.checkout = fetch.GclientCheckout(self.opts, {}, '/root') self.addCleanup(mock.patch.stopall) @@ -276,11 +222,8 @@ class TestGclientGitCheckout(unittest.TestCase): self.run_gclient = mock.patch('fetch.GclientCheckout.run_gclient').start() self.run_git = mock.patch('fetch.GitCheckout.run_git').start() - self.opts = optparse.Values({ - 'dry_run': False, - 'nohooks': True, - 'no_history': False - }) + self.opts = argparse.Namespace( + dry_run=False, nohooks=True, no_history=False) specs = { 'solutions': [{ 'foo': 'bar',