Add always and never matchers.

[mithrandi]

Fixes LP#947026. Please criticize ruthlessly for style / formatting / etc. since I just cribbed a bunch from other modules.

[jml]

Thanks so much! I've been meaning to do this for years, and am very grateful you made time to do it.

Keeping up the ruthlessness…

• please add an update in NEWS
• if you aren't already in AUTHORS and the "Thanks" in README.rst, please add yourself (I really should automate that)
• please update testtools/tests/matchers/__init__.py to include the new tests (did you watch them fail?)
• please mention these in the Combining matchers section of the for-test-authors.rst document, probably just inlining the API documentation as demonstrated in twisted-support.rst

Ping me when you're done, and I'll give it another pass & then merge.

[thomir]

Hi,

I wonder whether we need the _Always class and the always function? IIRC, all other matchers are title-cased, and most are classes (not that that distinction matters to the user). Is there a reason to break the established idiom of matchers appearing as classes in this case?

[jml]

@thomir We've started to change that idiom with the twistedsupport matchers. Basically, I don't want to export concrete types unless we have to. I've been meaning to write up my justification for this, but https://www.youtube.com/watch?v=3MNVP9-hglc is a good start.

[thomir]

We've started to change that idiom with the twistedsupport matchers. Basically, I don't want to export concrete types unless we have to.

Hmmmm, whether or not you're exporting a concrete type, it seems odd to me to break a naming convention. As an end user, if I want to import 'randommatcher', how do I know if I have to type:

from testtools.matchers import randommatcher

-or-

from testtools.matchers import RandomMatcher

?

I don't mind having a level of indirection via a function (that's a conversation for another day, at least), but breaking the titlecase naming convention seems like it's going to cause confusion and surprise.

[rbtcollins]

Review status: 0 of 3 files reviewed at latest revision, 10 unresolved discussions.

---

testtools/matchers/init.py, line 18 [r1] (raw file): This is inconsistent with everything else; I'm open to the idea that we should migrate to exporting functions, but doing it in dribs and drabs with no way for folk to predict will lower usability.

---

testtools/matchers/init.py, line 49 [r1] (raw file): Same.

---

_testtools/matchers/_const.py, line 16 [r1] (raw file):_ This is missing str, a requirement of matcher objects - see Matcher.

---

_testtools/matchers/_const.py, line 29 [r1] (raw file):_ Same, missing str

---

_testtools/tests/matchers/test_const.py, line 7 [r1] (raw file):_ FWIW we haven't previously done docstrings on test classes, and I generally consider them noise unless there is something unique about the domain being tested // manner of testing needed.

---

_testtools/tests/matchers/test_const.py, line 8 [r1] (raw file):_ Approximately none of our test methods have docstrings. I don't oppose having them, but I would oppose requiring them. Docstrings are for API documentation, and tests are not (generally) an API.

---

Comments from the review on Reviewable.io

[rbtcollins]

Review status: 0 of 3 files reviewed at latest revision, 11 unresolved discussions.

---

_testtools/tests/matchers/test_const.py, line 12 [r1] (raw file):_ One of the things missing is str as already noted - we need tests for that.

I"d also like to see tests for the mismatch returned by never, at least in a broad interface based sense.

---

Comments from the review on Reviewable.io

[rbtcollins]

Thanks for the patch; have reviewed - i have some detailed nits, but conceptually sure, its a nice thing to have.

[mithrandi]

Review status: 0 of 8 files reviewed at latest revision, 11 unresolved discussions.

---

testtools/matchers/init.py, line 18 [r1] (raw file): I suppose you and jml need to fight this one out ;)

This isn't inconsistent with everything else, though; raises is a function (although there is Raises too, which does something different), and more recently the deferred matchers also export functions rather than classes.

---

_testtools/matchers/_const.py, line 16 [r1] (raw file):_ Added __str__.

---

_testtools/matchers/_const.py, line 20 [r1] (raw file):_ Done.

---

_testtools/matchers/_const.py, line 29 [r1] (raw file):_ Fixed.

---

_testtools/matchers/_const.py, line 32 [r1] (raw file):_ FWIW, I thought about this a bit, but didn't come up with any great ideas.

---

_testtools/matchers/_const.py, line 33 [r1] (raw file):_ Done.

---

_testtools/tests/matchers/test_const.py, line 7 [r1] (raw file):_ I added some (brief!) narrative docstrings, hopefully that's not too much noise.

---

_testtools/tests/matchers/test_const.py, line 12 [r1] (raw file):_ I rewrote the tests to use TestMatchersInterface which seems much nicer; thanks for the pointer!

---

_testtools/tests/matchers/test_const.py, line 12 [r1] (raw file):_ I think this should now be covered by the new TestMatchersInterface-based tests.

---

Comments from the review on Reviewable.io

[jml]

@thomir We already go both ways on this (e.g. we have both raises and PathExists), so this isn't introducing a new problem. Perhaps it's exacerbating one. On balance, I would be OK with renaming these functions to have CamelCase names, since that seems to be the way we go most often in the code.

---

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.

---

testtools/matchers/init.py, line 18 [r1] (raw file): Right, there is both old & new precedent, so I'm OK with this change. As mentioned above, I'd be OK with renaming to CamelCase, but really don't think it's necessary.

If we don't migrate incrementally, then we can either not migrate at all or do so en masse. Migrating en masse is also a usability hit. Not migrating at all means writing worse Python, which makes our code base harder to maintain and is demotivating. Incremental migration is the least bad one, to my mind.

---

_testtools/matchers/_const.py, line 29 [r1] (raw file):_ Thanks

---

_testtools/matchers/_const.py, line 24 [r2] (raw file):_ Sorry, I may have mislead you. testtools docstrings don't have a newline after the initial """. I can make this change when merging if necessary.

---

_testtools/tests/matchers/test_const.py, line 8 [r1] (raw file):_ I said comments, fwiw, which many tests do have. Also, I disagree that docstrings are purely for API documentation. I am OK with such comments being optional.

---

_testtools/tests/matchers/test_const.py, line 12 [r1] (raw file):_ Thanks.

---

Comments from the review on Reviewable.io

[mithrandi]

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.

---

_testtools/matchers/_const.py, line 24 [r2] (raw file):_ No, that was just me switching between too many projects. Fixed.

---

Comments from the review on Reviewable.io

[rbtcollins]

Ok, so since thomi and I ere both asking for Always and Never, and @jml is ok with a rename on balance; could you please do that, and then I think its landable.

---

Reviewed 1 of 3 files at r1, 5 of 7 files at r2, 2 of 2 files at r3. Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

testtools/matchers/init.py, line 18 [r1] (raw file): By en mass being a usability hit, I presume you mean the fact that there would be a flag day, and folk would need to remember 'before X, Upper, after X, lower' ? True - but thats a single lookup table entry, not scaled by the number of matchers we add over the next N years.

w.r.t raises, I think in hindsight we should have done _Raises for the class and Raises for the thing people use. In the world of Nouns :).

---

_testtools/tests/matchers/test_const.py, line 8 [r1] (raw file):_ Sorry yeah, misread in the context of the request for a class docstring (which is also unusual in the test suite, but shrug).

---

_Comments from the review on Reviewable.io_

[mithrandi]

Okay, I uppercased the names.

---

Review status: 3 of 8 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from the review on Reviewable.io

[thomir]

I'm now +1 to land - thanks!

[jml]

Merged as f5a370e