openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

The setting to store the data directory should either be "data path" or "custom data path", not "new data path". Personally, I would go with "data path" and then just delete the value and let OpenLP do it's thing to create a default data path (though you may have to code this yourself anyway)

If you catch an exception you must *always* log that exception to the log file via "log.exception(u'Brief description of what went wrong')"

We use shutil for other copying actions, I recommend you use that too, just for consistency's sake.

AppLocation.IsDefaultDataPath should not be a class-level variable. Class-level "variables" are used as enumerations, and should never be altered programmatically. You should create a method instead, called "AppLocation.is_default_data_path()".

Line 435 and onward should look like this:

# Check if we have a different data location.
path = unicode(QtCore.QSettings().value(
    u'advanced/data path', QtCore.QVariant(u'')).toString())
if not path or not os.path.exists(path):
    path = AppLocation.get_directory(AppLocation.DataDir)
    check_directory_exists(path)

Additionally, your new method should look something like this:

@staticmethod
def is_default_data_path():
    path = unicode(QtCore.QSettings().value(
        u'advanced/data path', QtCore.QVariant(u'')).toString())
    return not path or not os.path.exists(path)

-- 
https://code.launchpad.net/~smpettit/openlp/data-path/+merge/89612
Your team OpenLP Core is subscribed to branch lp:openlp.