Author Alex.LordThorsen
Recipients Alex.LordThorsen, ghaering, jim_minter, r.david.murray
Date 2015-05-17.23:33:20
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1431905601.2.0.955841405003.issue16864@psf.upfronthosting.co.za>
In-reply-to
Content
There was a bunch of things wrong with that patch.

In addition to the issues you brought up in the review I was mixing up what the actual problem is

REPLACE INTO is an alias for INSERT OR REPLACE. INSERT OR REPLACE was correctly setting the lastrowid values but REPLACE INTO was not setting the last rowid value. I changed the documentation modifications to reflect this.

In addition I cleaned up the unit tests. The unit tests were kind of a mess because I was trying to figure out what the problem was and never refactored after getting a reproduction.

 I at first went down the path of making the tests use a for loop like you suggested

    for statement in ["INSERT OR REPLACE", "REPLACE"]:
        sql = "{} INTO test(id, unique_test) VALUES (?, ?)".format(
            statement)
        self.cu.execute(sql, (1, "foo"))
        self.assertEqual(self.cu.lastrowid, 1)
        self.cu.execute(sql, (1, "foo"))$
        self.assertEqual(self.cu.lastrowid, 1) 

Which I don't think is as nice as a cleaned up unrolled version

    self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)
    self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)
    self.cu.execute("REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)

I've created a patch that fixes all of the issues brought up in the code review and is just generally much cleaner.
History
Date User Action Args
2015-05-17 23:33:21Alex.LordThorsensetrecipients: + Alex.LordThorsen, ghaering, r.david.murray, jim_minter
2015-05-17 23:33:21Alex.LordThorsensetmessageid: <1431905601.2.0.955841405003.issue16864@psf.upfronthosting.co.za>
2015-05-17 23:33:21Alex.LordThorsenlinkissue16864 messages
2015-05-17 23:33:21Alex.LordThorsencreate