Re: [PATCH] raise Custom Exception on invalid test argument

Madan U Sreenivasan wrote:
>
> [[[
> Spew out a customized Exception when the test is provided with an
> invalid argument.
>
> * subversion/tests/cmdline/svntest/main.py
> (run_tests): Catch ValueError Exception and throw back
> a customised Exception that makes more sense to the user.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: svntest/main.py
> ===================================================================
> --- svntest/main.py (revision 19800)
> +++ svntest/main.py (working copy)
> @@ -808,7 +808,13 @@
> elif arg.startswith('BASE_URL='):
> test_area_url = arg[9:]
> else:
> - testnums.append(int(arg))
> + try:
> + testnums.append(int(arg))
> + except ValueError:
> + err_str = "Invalid argument :%s.\n" % arg
> + err_str += "Valid arguments are: "
> + err_str += "`list' and/or a valid test number."
> + raise Exception(err_str)

These last four lines can be just one statement, you know. Suggest:

   raise Exception("""Invalid argument: %s
Valid arguments are 'list' and/or a valid test number.""" % (arg))

Or, if the indentation bothers you:

   raise Exception("Invalid argument: %s\n" % (arg) + \
                   "Valid arguments are 'list' and/or a " + \
                   "valid test number.")

But beyond that, the valid arguments *aren't* just 'list' and a number,
because the very conditional you're modifying also allows for arguments that
begin with 'BASE_URL='. If the goal is to list all the arguments we accept,
you'll need to mention that one, too. If, however, the goal is to list the
argument we *want* to accept, then don't mention 'list' (because we'd prefer
folks to use the '--list' option instead).

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand