Code review comment for lp:~ralsina/ubuntuone-windows-installer/fix_809873

Revision history for this message
Manuel de la Peña (mandel) wrote :

Cool, We need to get this to lang, here are some comments:

* It looks to me that glob is not longer used, shall we remove the import.
* Regarding: os.system("bzr branch %s %s" % (urls[0], folder_name)) could we use Popen for that and find the location of the .exe via 'from distutils.spawn import find_executable'. That will also ensure that we can test if the system has bzr, if not we can give a meaningful error.
* In:
325 + # Remove "installed" copy
326 + try:
327 + shutil.rmtree(os.path.join("installed", "Lib"))
328 + except OSError:
329 + pass

Why do we ignore the exception?
* Within the source tree of ubuntuone-client there is a windows folder that contains an example loging.conf maybe we can simply copy it rather than doing:
401 + with open(logging_path, "rb") as f:
402 + data = f.read()
403 + data = data.replace("@LOG_LEVEL@", LOG_LEVEL)
404 + data = data.replace("@LOG_FILE_SIZE@", LOG_FILE_SIZE)
405 + with open(os.path.join("data", "logging.conf"), "wb") as f:
406 + f.write(data)
* Shall we put a poor mans name for:
449 + author='',
450 + author_email='',

review: Needs Fixing

« Back to merge proposal