Code review comment for lp:~flo-fuchs/mailman.client/settings

Revision history for this message
Barry Warsaw (barry) wrote :

This looks good, thanks! I have just two minor suggestions.

* Where you're returning a unicode string object in the doctests, it would be better to use 'print' so you don't get the ugly u'' prefixes. E.g., instead of:

>>> settings_new['description']
u'A very meaningful description.'

use:

>>> print settings_new['description']
A very meaningful description.

* You should not need to use the u'' strings in your doctests. Mailman does a 'from __future__ import unicode_literals' in all its .py files to force literals to be unicodes (in the absence of explicit b'' prefixes). mailman.client should do the same. To make this work in doctests, you need to set the testobj's globs in the doctest's setUp(). See src/mailman/tests/test_documentation.py down about line 183 for an example.

Other than that, it looks great, and please do merge it after consideration of the above. Thanks!

review: Approve

« Back to merge proposal