Bug#851359: diffoscope: Improve support for comparing images

[Chris Lamb]

Hey Maria,

> I tried to improve indentation and other cosmetic things. What do you
> think of the current state?

Thanks for the quick updates :)

So, I think…

    if get_image_size(self.path) != get_image_size(other.path):
        raise AssertionError

… are better written as:

    assert get_image_size(self.path) == get_image_size(other.path)

Or at least it is more Pythonic…  However, we should definitely *not*
be catching AssertionError like that for control flow, so either we need
to raise a different exception or find some other way of doing this.

For example, did you try farming all these checks out to another method?
That way you could have

    def fn(self, other):
        if get_image_size(self.path) != get_image_size(other.path):
            return False

        if other_check:
            return False

        return True


> +    # Store information about whether we are going to output html,
> +    # so we don't waste time computing things we aren't going to use.

Right, where "things" means calculating the visual diffs given that
only the HTML output currently supports them? If so:

  a) We should rename this global variable to something that reflects
     this usage.
 
  b) We could always just calculate the visual diffs and simply not
     render them in the presenter backends that do not support them.
     Are they remarkably slow, for example?

(Agree that making them lazy would not be the cleanest solution)


Regards,

-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      lamby at debian.org / chris-lamb.co.uk
       `-