Code review comment for lp:~mandel/ubuntuone-control-panel/auto-update-functions

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

A few minor tweaks:

* instead of duplicating the -i "test_windows.py" in run-tests, please define a new bash variable and use that

* we're trying to get rid of the authors section of the source headers, so instead of adding yourself, please remove all the authors in the files you modify :-)

* remember to always use %r for logging stuff

* why are we receiving the logger by parameter? that makes the api of the methods weird... let's grab the logger just like in the rest of the app (and then we can remove the fake logger altogether):

from ubuntuone.controlpanel import set_up_logger

logger = set_up_logger('utils')

* the logging itself could use some improvement, so when seen in a log file our of the code context, we can understand what information is giving us. So below I propose a new version of are_updates_present:

@defer.inlineCallbacks
def are_updates_present():
    """Return if there are updates for Ubuntu One."""
     result = False
     retcode = None
    update_path = _get_update_path()
    if update_path is not None:
        # If there is an update present we will get 0, non-zero otherwise
        retcode = yield getProcessValue(update_path, args=('--mode',
            'unattended'), path=os.path.dirname(update_path))
        result = retcode == 0
    logger.debug('are_updates_present: update path: %r, return code: %r, result: %r',
                 update_path, retcode, result)
    defer.returnValue(result)

* Remember to have cleanup code to be run ALWAYS, even if the test fails. In your tests, the cleanup code is only run if none of the asserts failed. So, please use self.addCleanup calls as soon as you modify the env for the test.

review: Needs Fixing

« Back to merge proposal