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/+/3541479Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Aravind Vasudevan <aravindvasudev@google.com>

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] <config> [--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 --<name>=<value>
+  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__':

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',