Code review comment for lp:~henninge/launchpad/devel-710591-sharing-info-groundwork-0

Revision history for this message
Deryck Hodge (deryck) wrote :

Henning,

This looks good. Thanks for taking the time to jump on Mumble and talk this over with me. I really like the test, and appreciate you being thorough in covering all the various cases.

Some minor cleanups/clarifications could help:

 * Remove the pylint disable-msg declaration
 * Be consistent about comments on the test methods
    (Just so it's easy to scan the test and work out
     what is being tested)
 * Add clarify comments to the main module (docstring perhaps?)

This last one is because it was not clear to me where I would use these methods, or why I should prefer one over the other. So some documentation to clarify would help. And also prevent people importing all these functions and doing:

    if has_upstream_template():
        info = get_upstream_sharing_info()

To help prevent people firing off more queries then they they realize.

Thanks, again!

review: Approve (code)

« Back to merge proposal