openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for the contribution! We're really glad to see you helping out!

This looks good, but there are two fixes I would like you to make:

Firstly, according to our style guide, and according to PEP8, you should not have a space before the : in your elif.

Secondly, it would be better, rather than repeating code unnecessarily, to check if you're dealing with a MySQL database within the "else" and after you have set self.db_url. Something like this:

else:
    self.db_url = u'%s://%s:%s@%s/%s' % (db_type,
        urlquote(unicode(settings.value(u'db username').toString())),
        urlquote(unicode(settings.value(u'db password').toString())),
        urlquote(unicode(settings.value(u'db hostname').toString())),
        urlquote(unicode(settings.value(u'db database').toString())))
    if db_type == u'mysql':
        self.db_url += u'?charset=utf8'
-- 
https://code.launchpad.net/~rapecik/openlp/bug-941692/+merge/94702
Your team OpenLP Core is subscribed to branch lp:openlp.