divmod-dev team mailing list archive

[Jeremy Thurgood <firxen@xxxxxxxxx>]

I'm not sufficiently familiar with the codebase to do a thorough review, but I did notice a few small things:

 * ExpressContents.contents() returns a list of sentence fragments or an empty string. Given that the docstring explicitly mentions returning a list, I think the final line should be `return []` (or maybe `return [u""]` if that makes more sense) rather than `return u""`.

 * ConceptTemplateTests doesn't have a test case for multiple substitutions or format specifiers that aren't at the start of the string. A test for something like u"foo {c:pronoun}{c:name} bar {c:pronoun}" would hit a bunch of edge cases in the formatter.

 * ExpressContentsTests.test_template() uses a valid but incorrect template to test with. That's okay for the test, since the template isn't being expanded at all, but it's probably better to have a correct template in the test.

 * You could probably move ExpressContentsTests.addContents() up a bit and use it in some earlier tests.
-- 
https://code.launchpad.net/~exarkun/divmod.org/explained-inventory-1193494/+merge/172932
Your team Divmod-dev is requested to review the proposed merge of lp:~exarkun/divmod.org/explained-inventory-1193494 into lp:divmod.org.