openlp-core team mailing list archive

[Jonathan Corwin <j@xxxxxxxxxxxx>]

On Sat, May 2, 2009 at 9:31 PM, Raoul Snyman <
raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx> 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.

-- 
https://code.launchpad.net/~j-corwin/openlp/migration/+merge/6124
Your team openlp.org Core is subscribed to branch lp:openlp.