Greg Stein <gstein@lyra.org> writes:
> On Tue, Mar 18, 2003 at 03:30:21PM -0600, fitz@tigris.org wrote:
> >...
> > +++ trunk/subversion/tests/clients/cmdline/prop_tests.py Tue Mar 18 15:2
> 9:54 2003
> >...
> > @@ -72,7 +69,7 @@
> > expected_status.tweak('A/D/G', status=' M')
> >
> > if svntest.actions.run_and_verify_status(wc_dir, expected_status):
> > - return 1
> > + raise svntest.Failure
>
> Woah! The idea is to *simplify* the test functions. This doesn't simplify
> the stuff at all -- it just changes how the function has to handle an error.
> Instead, run_and_verify_status() should raise the exception. The test
> function would just do:
>
> svntest.actions.run_and_verify_status(wc_dir, expected_status)
>
> Of course, to do this, you'll need to see if anybody analyzes the return
> value from that function, or if they just use the "if test: return 1"
> pattern everywhere.
>
> But just switching "return 1" to "raise svntest.Failure" doesn't move us
> forward. -1 on that design, sorry :-(
Woah woah woah woah woah easy there Gunga Din. I appreciate the
review, but this is *in progress* here. I fully intend to convert all
the functions/methods inside svntest to raise on error instead of
return a non-zero value. My fault for not making this clear. :)
After the above commit, I talked it over with Karl and determined
that, in order to do this bit by bit in reviewable hunks that I would
next convert the stuff inside svntest that currently returns 0 or
non-zero to return 0 or raise. Then I could leisurely convert each
file in the cmdline directory to the format that you described above,
and once that was done, remove the spurious returns from those
functions in svntest. Does that make sense?
> > - raise svntest.main.SVNTreeUnequal
> > + raise svntest.Failure
>
> Woah! Losing information. Create a subclass of svntest.Failure instead. Oh
> hey... lookee there. SVNTreeUnequal *is* a subclass of svntest.Failure :-)
Gotcha. Will revert.
> >...
> > # These should produce an error
> > outlines,errlines = svntest.main.run_svn('Illegal target', 'propset',
> > 'svn:executable', 'on',
> > A_path)
> > if not errlines:
> > - return 1
> > + raise svntest.Failure
>
> Okay. Now these are legitimate raises within the test functions. :-)
>
> But you should still define a custom exception:
>
> class ExpectedErrorOutput(svntest.Failure):
> "docco..."
> pass
Do we want a custom class for everything? Why can't we just raise with
a description like:
raise svntest.Failure, "Blah blah blah"
And handle that appropriately. Or is it more Pythonesque to have a
zillion Exception classes.
> >...
> > @@ -741,14 +730,12 @@
> > out, err = svntest.main.run_svn(None, 'propset', '--revprop', '-r', '0',
> > 'cash-sound', 'cha-ching!', sbox.wc_dir)
> > if err:
> > - return 1
> > + raise svntest.Failure
>
> I think that we want a run_svn() variant that raises exception if anything
> is ever produced on stderr. Maybe run_svn_noerr().
Hmmm.
OK. Now off to extract a gstein from my throat.
-Fitz
--
Brian W. Fitzpatrick <fitz_at_red-bean.com> http://www.red-bean.com/fitz/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 19 01:05:52 2003