Re: [Zeitgeist] [Merge] lp:~mhr3/zeitgeist/bb-schema-ver-table into lp:~zeitgeist/zeitgeist/bluebird
Review: Approve
The line «throw new EngineError.DATABASE_ERROR ("Unable to upgrade from schema»
is longer than 80 chars. And there's a typo in my comment ("appriopriate" ->
"appropriate") :P.
--
https://code.launchpad.net/~mhr3/zeitgeist/bb-schema-ver-table/+merge/79928
Your team Zeitgeist Framework Team is subscribed to branch
lp:~zeitgeist/zeitgeist/bluebird.
___
Mailing list: https://launchpad.net/~zeitgeist
Post to : zeitgeist@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zeitgeist
More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~mhr3/zeitgeist/bb-schema-ver-table into lp:~zeitgeist/zeitgeist/bluebird
Review: Needs Fixing Hey! Looks really nice. Here's some pedantic comments: - Shouldn't we abort if the database backup can't be created? - It'd make more sense for get_schema_version to be private, I don't think it's needed anywhere outside. - Can you please preserve the comment we had in Python before the "INSERT INTO schema_version VALUES" query? Otherwise it's rather confusing. # The 'ON CONFLICT REPLACE' on the PK converts INSERT to UPDATE # when appriopriate - "if (values != null && values[0] != null)" => I don't think 'values` can ever be null (what would happen instead is that the callback isn't called), so the first half of the check is empty. - "* © 2011 Canonical Ltd." => in the other files we duplicate the "Copyright" at the start of each statement. Consistency doesn't hurt :P. -- https://code.launchpad.net/~mhr3/zeitgeist/bb-schema-ver-table/+merge/79928 Your team Zeitgeist Framework Team is subscribed to branch lp:~zeitgeist/zeitgeist/bluebird. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~mhr3/zeitgeist/bb-schema-ver-table into lp:~zeitgeist/zeitgeist/bluebird
Review: Approve I like it a lot... Are we sure we don't want to upgrade from anything where core_schema < 4 ? Other than that +1 -- https://code.launchpad.net/~mhr3/zeitgeist/bb-schema-ver-table/+merge/79928 Your team Zeitgeist Framework Team is subscribed to branch lp:~zeitgeist/zeitgeist/bluebird. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
