Code review comment for lp:~raj-abhilash1/mailman/bug_1423756

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

Here's some general feedback as I work through the merge:

* Be careful about single quotes vs. double quotes. In general, I prefer single quotes except for multiline strings (including docstrings) and of course any string with an apostrophe in it.

* I fixed the interface for IDomainManager.add() - it still mentioned owners_id.

* You might want to use a Python linter to check for basic problems. E.g. I use pyflakes (hooked up to Emacs) so I noticed immediately some unused imports.

* Don't use mutables (e.g. []) in argument lists. If the mutable were to change, it would affect *every* call to the function. Use and check for `None` instead.

* You don't need to `assert isinstance(owners, list)` because we should accept any sequence. Probably not worth type checking the argument, as iteration ought to work, but of course it'll do the wrong thing if say owner is a string. That's okay, "we're all adults here" :) (Also, assert is not a function call so it doesn't need parentheses.)

* Implementations of interface methods can just have their docstring refer to the interface.

* I left a note in Domain.add_owners(). While I think the API is good, we might at some later point want to make that a bit more efficient than just calling add_owner(). Maybe.

More to come as I continue with the merge.

« Back to merge proposal