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.
--
Mailing list: https://launchpad.net/~divmod-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~divmod-dev
More help : https://help.launchpad.net/ListHelp
