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

On Sat, May 2, 2009 at 9:31 PM, Raoul Snyman <
<email address hidden>> wrote:

> Review: Needs Fixing
> Muhahaha! The Style Police has arrived!
>
> (Yes, this is mostly old code, but it needs to be said!)
>
> All strings are single-quoted and unicode:
>
> self.display.output("Songs processing started")
>
> should be:
>
> self.display.output(u'Songs processing started')

Was already there, nowt to do with me

>
> Why are you adding columns one at a time?
>
> conn.execute("""create table authors (id integer primary key ASC
> AUTOINCREMENT);""")
> conn.commit()
> self.display.sub_output("authors table created")
> conn.execute("""alter table authors add column first_name
> varchar(128);""")
> conn.commit()
> self.display.sub_output("first_name added")
> conn.execute("""alter table authors add column last_name varchar(128);""")
> conn.commit()
>
> Shouldn't that rather be:

> conn.execute("""
> create table authors (
> id integer primary key ASC AUTOINCREMENT,
> first_name varchar(128),
> last_name varchar(128),
> display_name varchar(255)
> );""")
> conn.commit()
> self.display.sub_output("authors table created")

I agree, but again was just copying what was already there since I didn't
know if the original author had done it that way on purpose

>
> Also name variables better:
>
> # I'm not sure what these two variables are supposed to be
> def run_cmd(self, cmd):
> f_i, f_o = os.popen4(cmd)

I didn't touch this function and have no idea what it does.

All I was doing was updating existing code so it worked... I wasn't planning
on rewriting the whole existing
module.