Code review comment for lp:~jml/testtools/unprintable-assertThat-804127

Revision history for this message
Martin Packman (gz) wrote :

This looks like really the best option, thanks.

+try:
+ to_text = unicode
+except NameError:
+ to_text = str
...
- return unicode(evalue)
+ return to_text(evalue)

I think this is abstraction at the wrong level, Python 3 should just be able to do `str(e)` rather than `_exception_to_text(e)` where you use it, or if we really want the exception catching (doesn't seem needed in the test), add a Python 3 version which would basically be:

    def _exception_to_text(evalue):
        try:
            return str(evalue)
        except Exception:
            return ...some fallback...

+ def __init__(self, matchee, matcher, mismatch, verbose=False):

I'm not sure about having verbosity as an attribute of the exception instance, but guess that's a natural consequence given the way the option was added earlier. Otherwise, the traceback formatting code would be complicated by trying to call something other than unicode()/str() on the exception so a verbose argument could be passed. Which would get very ugly.

+ def __str__(self):
+ difference = self.mismatch.describe()

If we are strict about describe methods always returning unicode, and don't expect MismatchError instances to be handled outside the testtools module, this is okay. Otherwise, it would be better to have both a __unicode__ and __str__ method, with the __str__ method returning ascii on Python 2. I can put up some testcases demonstrating the issues involved.

+ # XXX: Using to_text rather than str because, on Python 2, str will
+ # raise UnicodeEncodeError.

As mentioned above, this may be worth avoiding by having __str__ fall back to ascii on Python 2.

+ if ``self.failureException`` has been set to a non-default value, then
+ mismatches will become test errors rather than test failures.

Not that I've ever seen anyone use this unittest feature, but could the testtools runners at least avoid this by doing `except (self.failureException, MismatchError):` or similar?

review: Approve

« Back to merge proposal