[MERGE] Default InterRepository.fetch raises IncompatibleRepositories

[Martin Pool]

On Tue, Jul 29, 2008 at 8:56 AM, John Arbash Meinel
<john at arbash-meinel.com> wrote:

Thanks for the review.

> | +    @staticmethod
> | +    def _assert_same_model(source, target):
> | +        """Raise an exception if two repositories do not use the same
> model.
> | +        """
> |          if source.supports_rich_root() != target.supports_rich_root():
> | -            return False
> | +            raise errors.IncompatibleRepositories(source, target,
> | +                "different rich-root support")
> |          if source._serializer != target._serializer:
> | -            return False
> | -        return True
> | +            raise errors.IncompatibleRepositories(source, target,
> | +                "different serializers")
> |
>
> ^- This seems like it might be better layered as _assert_same_model
> raising an exception based on the return value of _same_model. So rather
> than catching the exception and returning False, you do:
>
> if not ..._same_model():
> ~  raise IncompatibleRepositories
>
> Is it just that _assert has better details about *why*? You could return
> that from _same_model.

Yes, it is that.  You're right that we could e.g. make _same_model
return a tuple of (is_same, reason_why_not), but it seemed to me that
most places where it was different would end up raising this exception
so we might as well do it directly.  Also, _same_model is already
defined to just return a boolean so we'd have to add a new api anyhow.

(If it was the true-case where we wanted more detail we could fudge it
but there's no way to put details into a false value.  OK, you could
have a custom class that acts as false but contains other things but
that's getting silly.)

-- 
Martin <http://launchpad.net/~mbp/>