Explicit interfaces for matchers and mismatches #211
Conversation
| Describes the protocol that mismatches are required to implement. | ||
| """ | ||
|
|
||
| # XXX: jml would like to extend this interface to include the thing that |
There was a problem hiding this comment.
FWIW, I'm +1 on this - I frequently find myself writing a test that fails, then adding the matchee as the third parameter to the assertion so it gets printed out, then fixing the test and removing that third parameter. Annoying. I'm not sure if that should go in the matchers themselves, or in (assert|expect)That.
There was a problem hiding this comment.
Ah, interesting. I mostly want it for composition, e.g. to help with Not. ISTR wanting it for https://github.com/ClusterHQ/flocker/blob/master/flocker/testtools/matchers.py but looking at it, I can't see what was essential.
Whether we or not we extend the interface this way, I want second opinions on whether it should be done in this patch / this release.
There was a problem hiding this comment.
@rbtcollins Any thoughts on this?
There was a problem hiding this comment.
@jml Just for extra clarity, does "include the thing that" mean "the matchee argument passed to match()"?
From a very quick look at the code it seems that implementations of IMatcher typically pass or could pass the matchee object to the constructor of an IMismatch, in order to have describe() take it into account.
Why do we need to pass it to describe()?
An example of the "composition" use case you allude to would be appreciated.
There was a problem hiding this comment.
does "include the thing that" mean "the matchee argument passed to match()"
Yes.
Why do we need to pass it to describe()?
Did I propose this?
An example of the "composition" use case you allude to would be appreciated.
Sorry, the intervening 13 months have somehow scrubbed it from my memory. However, looking at the file linked to in my comment provides just such an example. You'll have to figure out what it does, I'm afraid.
|
A few comments above. Probably worth getting @rbtcollins opinion as well, but I'm +1 anyway. |
|
Thanks. I'll wait for @rbtcollins, because he had opinions about #181. |
|
|
||
| :param matchee: An object to match with ``matcher``. | ||
| :param IMatcher matcher: An object meeting the testtools.Matcher protocol. | ||
| :raises MismatchError: When ``matcher`` does not match ``matchee``. | ||
|
|
|
This looks much saner to me. |
* Adds `IMatcher` and `IMismatch` * Updates their documentation as well as that in `assert_that`, `assertThat`, and `expectThat` to refer to these * Changes `MismatchDecorator` to explicitly subclass `IMismatch`
|
Updated based on review comments. Need clarification on a couple of points before I land. |
|
Don't want this on my Github dashboard. Others are free to re-submit if they'd like. |
|
Jonathan Lange <notifications@github.com> writes:
jml commented on this pull request.
> @@ -53,7 +62,49 @@ def __str__(self):
raise NotImplementedError(self.__str__)
-class Mismatch(object):
+Matcher = IMatcher
+
+
+class IMismatch(object):
+ """A mismatch detected by an IMatcher.
+
+ Describes the protocol that mismatches are required to implement.
+ """
+
+ # XXX: jml would like to extend this interface to include the thing that
> does "include the thing that" mean "the matchee argument passed to match()"
Yes.
> Why do we need to pass it to describe()?
Did I propose this?
I might have misunderstood. However what does
```python
# XXX: jml would like to extend this interface to include the thing that
# failed to match.
```
refer to then? If you recall. IOW how would such an extension look like.
|
|
A public Regrettably, this is my last update to this PR. I don't use Python at home and I don't use testtools at work, so I'm not going to make time to pursue this further. |
|
@jml thanks for the input |
IMatcherandIMismatchassert_that,assertThat, andexpectThatto refer to theseMismatchDecoratorto explicitly subclassIMismatchAnother take on #181. This time, just using Python.
While this patch is not a huge improvement in and of itself, I want to start doing this (explicit abstract types) for the rest of testtools. I think cumulatively it will make a great improvement to the code base. (Already the work on #178 has helped me clarify much). Matchers are an easy place to start because they are so close to this already.