Code review comment for lp:~j-corwin/openlp/migration

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Yes, I know it's not your code, but I'm just saying this for the record...

Use the os.path module functions, they tend to take care of little things:

  conn = sqlite3.connect(self.data_path + os.sep + database)

  should rather be:

  conn = sqlite3.connect(os.path.join(self.data_path, database))

Indices really should have functional names... "author1" is not a good name:

  conn.execute("""create index if not exists author1 on authors (display_name ASC,id ASC);""")

  better:

  conn.execute("""create index if not exists author_display_name on authors (display_name ASC,id ASC);""")

I'm a little wary of all these "replace"s - there's probably a better way...

  replace(replace(replace(replace(replace(replace(replace(replace(

review: Approve (approve)

« Back to merge proposal