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

Revision history for this message
Jonathan Lange (jml) wrote :

Heroic effort. I can only imagine what this must have been like to write.

What follows is initial feedback and thoughts. Answer & fix as much as you
like. I'm happy to fix up the minor stuff as I land.

To summarize, although the bug was originally filed against assertThat, it
turns out that it's actually an issue in the matchers themselves as well as
assertThat. Right?

If so, we have to be thinking about:
 * what are our failure modes with this code and existing broken matchers
 * how to make it easy to define matchers and mismatches correctly

Will return to those in a bit. General code stuff:

 * Can you elaborate on your comment on istext()?
 * Is there a way we can make it so matchers never have to use _isbytes?
   Perhaps by adding an "is_multiline" function to compat.
 * I don't care about astral characters
 * In compat, "Seperate" should be spelled "Separate"
 * text_repr is a bit hard to follow, maybe add some internal comments?
 * Some more documentation on the intent & API of text_repr would help,
   saying that it attempts to mimic repr() except returning unicode,
   giving some examples, explaining why you might want to use it and also what
   multiline does.
 * I don't get why multiline needs three different ways of escaping lines
   whereas the non-multiline case only has two branches
 * I think having a repr()-like function that takes any object and
   special-cases text would be good
 * Just to be clear, we are expecting Mismatch.describe() to return unicode,
   right?
 * What are we expecting Matcher.__str__ to return? Text or bytes?

On the original points:

 * We should add something to the docs explaining best practice for
   Mismatch.describe() methods. i.e., do "foo %s" % (text_repr(x),)
 * We should update the docstring for ``Mismatch.describe``
 * Our docs say that match() should return a Mismatch, so we can assume
   subclassing of that.
 * These docs need to bubble up, to AnnotatedMismatch, to assertThat etc.
   e.g. The 'message' parameter of assertThat needs to have its type
   documented.

Thanks so much for persevering.

jml

« Back to merge proposal