Fix instance based layers #20
Conversation
(Test layers themselves don't need to explicitly indicate that they provide this interface.)
Commit 3a68ee9 improved layer sorting but accidentally dropped support for instance-based layers. These turn out to be rather widely used (in ZODB, in zope.app.testing, in plone.testing etc). This change fixes the above problem in a different way from PR #6: it replacing inspect.getmro(layer) with a recursive sort key computation that traverses layer.__bases__. It doesn't actually use the C3 algorithm for MRO computation. I tried to find an example where that would matter and couldn't. This commit also adds tests to prevent this from regressing in the future.
| "Dotted name of the module that defines this layer") | ||
|
|
||
| def __hash__(): | ||
| """A test layer must be hashable""" |
There was a problem hiding this comment.
Does this need an __eq__ as well? The two pretty much have to be defined using the same semantics, or else things break.
There was a problem hiding this comment.
I wasn't sure if I should include eq in the interface:
- On one hand, to be properly hashable an object has to have eq and hash and the two have to be consistent with each other.
- On the other hand Python object instances (including classes themselves) are identity-comparable and hashable by default, so the user doesn't have to do anything (unless they decide to override eq).
So I chose a compromise and documented one of the two, thus combining the downsides.
I think you're right, a reminder about eq would be good. (And also a comment that the default implementations provided by Python suffice.)
There was a problem hiding this comment.
I tried to add eq and it felt wrong to me again.
I don't oppose documenting that, but I can't find the words myself.
There was a problem hiding this comment.
I don't really care which, but we should either document both __hash__ and __eq__, or neither.
|
LGTM |
Commit 3a68ee9 improved layer sorting but accidentally dropped support for instance-based layers. These turn out to be rather widely used (in ZODB, in zope.app.testing, in plone.testing etc).
This change fixes the above problem in a different way from PR #19: it replacing inspect.getmro(layer) with a recursive sort key computation that traverses layer.bases.
It doesn't actually use the C3 algorithm for MRO computation. I tried to find an example where that would matter and couldn't.
This change also adds tests to prevent this from regressing in the future and defines the test runner's expectations for test layers as interfaces.