Applying logical OR to a constraint "fails" silently

[wjbenfold]

✨ Feature Request

Whilst answering a recent support query, it was highlighted to me that currently applying logical OR to two constraints effectively fails silently.

Motivation

We could change Constraint to throw a NotImplementedError. Pros: people don't have a silent failure. Cons: currently functional code could start throwing exceptions.

Which should we do? Change it or leave it?

Additional context

The lack of an OR operator on them means that python falls back to its default behaviour of checking for truth in the first argument, returning it if it's "truthy", and if not returning the second argument

[wjbenfold]

I've experimented a bit and I think I was wrong above - at least in what I meant. Using "and" or "or" will fail silently, but the thing that we'd be spotting to catch this is Constraint being cast to a bool. Using "&" works as expected and "|" fails with a TypeError.

Given our docs don't say anything about "and" or "or" being ok, maybe it's just that one user thought that would be ok, rather than something that should be changed.

[trexfeathers]

The @SciTools/peloton have agreed that the Constraint class should include the appropriate override to prevent it being used in boolean statements - there is an accepted way of doing this (needs looking up, maybe __bool__?) - to raise TypeError. A unit test for this error should also be added.

This is not considered a breaking change since any code it breaks is using an anti-pattern.

The docs are already clear on the correct use of & instead.

[pp-mo]

the appropriate override to prevent it being used in boolean statements

After a little searching, I think raising TypeError from __bool__ is the "correct" way.
But I think providing a custom explanatory text is also very useful.
So, something like this:

>>> class MyClass:
...   def __bool__(self):
...     raise TypeError('Cannot use MyClass as a truth value.')
... 
>>> myc = MyClass()
>>> print('yes' if myc else 'no')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __bool__
TypeError: Cannot use MyClass as a truth value.
>>> 

For reference, this is how numpy generates its familiar message The truth value of an array with more than one element is ambiguous. Use a.any() or a.all() : here

Having said that, I think their use of ValueError instead of TypeError may be historical, and actually not quite right
- as explained here

(except...
maybe ValueError is ok, since they currently allow an empty array to be 'False',
though that is now deprecated

>>> print('YES' if np.arange(2)[:0] else 'NO')
<stdin>:1: DeprecationWarning: The truth value of an empty array is ambiguous. Returning False, but in future this will result in an error. Use `array.size > 0` to check that an array is not empty.
NO
>>>

)