Two-Dimensional Expression Layout

[Steve D'Aprano]

Oh wait! The penny drops!

On Sun, 21 Aug 2016 10:43 am, Michael Selik wrote:

> On Sat, Aug 20, 2016 at 6:21 PM Lawrence D’Oliveiro

>> >>     if (
>> >>             not isinstance(src, Image)
>> >>         or
>> >>             mask != None and not isinstance(mask, Image)
>> >>         or
>> >>             not isinstance(dest, Image)
>> >>     ) :
>> >>         raise TypeError("image args must be Image objects")
>> >>     #end if
>> >>
>> >
>> > No need for the separate calls to isinstance, nor the check for None.
>> >
>> >     if any(not isinstance(obj, Image) for obj in [src, mask, dest]):
>> >         ...
>>
>> Spot the bug in your version...
>>
> 
> It'd be easier if I ran the code :-)
> Let's see...
> - the ``or`` translates to an ``any(...)``
> - ``not isinstance(obj, Image)`` is repeated 3 times
> - ``[src, mask, dest]`` corresponds to the 3 objects
> - ``mask != None`` is unnecessary, unless somehow None has been registered
> as an instance of Image.
> 
> ... can't spot it. Give me a hint?

I think you've got the mask != None bit backwards. In Lawrence's version, if
mask is None, *no exception is raised*. In yours, it raises.

I think that it would be easier to reason about this code if it were written
in terms of positive assertions ("this condition is true") rather than
negative ("this condition is not true").

- src must be an Image;
- mask must be None or an Image;
- dest must be an Image.

Assuming I check for this in at least two places, I'd factor it out into a
helper function. There are probably a thousand ways to write it, and I'm
not married to any of them. Here are a couple:


def _validate(src, mask, dest):
    if mask is None or isinstance(mask, Image) and \
    all(isinstance(o) for o in [src, dest]):
        return
    raise TypeError("image args must be Image objects")


def _validate(src, mask, dest):
    if not (
            isinstance(src, Image) and
            mask is None or isinstance(mask, Image) and 
            isinstance(dest, Image)
           ):
        raise TypeError("image args must be Image objects")


The logic is *just* tricky enough that if this were my code I would
absolutely insist on having unit tests for this helper. Beyond that, the
specific details of how you test them is not that important, but given that
there are only three arguments to be tested, I'm inclined to test each one
individually and give a more useful error message:

def _validate(src, mask, dest):
    if not isinstance(src, Image):
        raise TypeError("src must be an Image")
    if mask is not None and not isinstance(mask, Image):
        raise TypeError("mask must be None or an Image")
    if not isinstance(dest, Image):
        raise TypeError("dest must be an Image")




-- 
Steve
“Cheer up,” they said, “things could be worse.” So I cheered up, and sure
enough, things got worse.