Thanks for all the feedback. I've just submitted a new combined patch with
the suggested test case included (makes sense to me to include it).
Cheers,
Christian
On Fri, Mar 19, 2010 at 5:45 PM, Vinzent Steinberg <
vinzent.steinb...@googlemail.com> wrote:
> We have the raises() helper function, look in the tests for it. See for
> example:
>
> x = Symbol('x')
> raises(TypeError, "len(x)")
>
> It usually makes sense to have a test that proves your patch actually
> works. Here you could for example add a test that would lead without
> your patch to an "ambiguous outcome" and now raises an exception
> instead. I agree that such a test is not very important, but even just
> an example in the commit message would already make review easier.
>
> There is no policy for testing exceptions, you should do it if you
> think it makes sense. There is however a policy that every new
> functionality has to be tested. In this case I would be fine if there
> is no test (I leave this up to you), but your commit message could
> explain the "ambiguous outcome".
>
> Thanks!
>
> Vinzent
>
> 2010/3/19 Christian Muise <christian.mu...@gmail.com>:
> > Not sure what such a test would look like...other oddities are left
> > unhandled such as Not(). What's the policy for sympy on testing thrown
> > errors?
> >
> > Cheers
> >
> > On Fri, Mar 19, 2010 at 4:53 PM, Vinzent Steinberg
> > <vinzent.steinb...@googlemail.com> wrote:
> >>
> >> 2010/3/17 Christian Muise <christian.mu...@gmail.com>:
> >> > ---
> >> > sympy/logic/boolalg.py | 6 ++----
> >> > 1 files changed, 2 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/sympy/logic/boolalg.py b/sympy/logic/boolalg.py
> >> > index 0a45e42..97c47b7 100755
> >> > --- a/sympy/logic/boolalg.py
> >> > +++ b/sympy/logic/boolalg.py
> >> > @@ -109,10 +109,8 @@ class Implies(BooleanFunction):
> >> > """
> >> > @classmethod
> >> > def eval(cls, *args):
> >> > - if len(args) < 2:
> >> > - raise ValueError, "Only %d operand(s) used for an Implies
> >> > (pairs are required): %s" % (len(args), str(args))
> >> > - elif len(args) > 2:
> >> > - return map(cls, args)
> >> > + if len(args) != 2:
> >> > + raise ValueError, "%d operand(s) used for an Implies
> (pairs
> >> > are required): %s" % (len(args), str(args))
> >> > else:
> >> > return Or(Not(args[0]), args[1])
> >> >
> >> > --
> >> > 1.6.3.3
> >>
> >> Would it make sense to add a test?
> >>
> >> Vinzent
>
> --
> You received this message because you are subscribed to the Google Groups
> "sympy-patches" group.
> To post to this group, send email to sympy-patc...@googlegroups.com.
> To unsubscribe from this group, send email to
> sympy-patches+unsubscr...@googlegroups.com<sympy-patches%2bunsubscr...@googlegroups.com>
> .
> For more options, visit this group at
> http://groups.google.com/group/sympy-patches?hl=en.
>
>
--
You received this message because you are subscribed to the Google Groups
"sympy-patches" group.
To post to this group, send email to sympy-patc...@googlegroups.com.
To unsubscribe from this group, send email to
sympy-patches+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/sympy-patches?hl=en.
