Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(150)

#16864: sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 6 months ago by jim
Modified:
2 years, 4 months ago
Reviewers:
rdmurray
CC:
gh_ghaering.de, r.david.murray, devnull_psf.upfronthosting.co.za, berkerpeksag, jim_minter.uk, Alex.Lord
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 #

Patch Set 3 #

Total comments: 5

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/sqlite3.rst View 1 2 3 4 5 1 chunk +13 lines, -3 lines 0 comments Download
Doc/whatsnew/3.6.rst View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download
Lib/sqlite3/test/dbapi.py View 1 2 3 4 5 2 chunks +83 lines, -1 line 0 comments Download
Modules/_sqlite/cursor.c View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 2
r.david.murray
http://bugs.python.org/review/16864/diff/11663/Lib/sqlite3/test/dbapi.py File Lib/sqlite3/test/dbapi.py (right): http://bugs.python.org/review/16864/diff/11663/Lib/sqlite3/test/dbapi.py#newcode529 Lib/sqlite3/test/dbapi.py:529: #test curser values for replace, which should be synonymous ...
2 years, 6 months ago #1
r.david.murray
2 years, 5 months ago #2
http://bugs.python.org/review/16864/diff/14837/Doc/library/sqlite3.rst
File Doc/library/sqlite3.rst (right):

http://bugs.python.org/review/16864/diff/14837/Doc/library/sqlite3.rst#newcod...
Doc/library/sqlite3.rst:630: 
ReST indentation is three spaces, and this has semantic significance.  YOu need
to have the '..' aligned under the 'called' above it.  You also have this
paragraph duplicated :)

"is now provides" should be "now provides"

http://bugs.python.org/review/16864/diff/14837/Doc/whatsnew/3.5.rst
File Doc/whatsnew/3.5.rst (right):

http://bugs.python.org/review/16864/diff/14837/Doc/whatsnew/3.5.rst#newcode484
Doc/whatsnew/3.5.rst:484: 
There should be a blank line after the ----, and you've got duplication again. 
I think it would also be clearer to say "now provides...for ``INSERT AND
REPLACE``.  INSERT is an existing feature and doesn't need to be mentioned here.

http://bugs.python.org/review/16864/diff/14837/Lib/sqlite3/test/dbapi.py
File Lib/sqlite3/test/dbapi.py (right):

http://bugs.python.org/review/16864/diff/14837/Lib/sqlite3/test/dbapi.py#newc...
Lib/sqlite3/test/dbapi.py:190: self.cu.execute(sql_string)
You don't need the sql_string later, so write this instead like this:

  self.cu.execute("create table test(id integer primary key, name text,"
                  " income number, unique_test text unique)")

(Putting the blank at the start of the continued string rather than the end of
the previous line is a piece of my own style, not something suggested by PEP8,
just FYI.)

http://bugs.python.org/review/16864/diff/14837/Lib/sqlite3/test/dbapi.py#newc...
Lib/sqlite3/test/dbapi.py:580: self.assertEqual(self.cu.lastrowid, 2)
I'm not clear on what behavior these additional tests are testing.  Reading the
module docs and the sqlite docs for last row id, it seems like this is returning
the unchanged value (ie: no row was modified)?  If so, it might be a good idea
to mention this in the docs, since it isn't clear that this is the case unless
you read the sqlite docs.  (Assuming I'm understanding this!)

http://bugs.python.org/review/16864/diff/14837/Modules/_sqlite/cursor.c
File Modules/_sqlite/cursor.c (right):

http://bugs.python.org/review/16864/diff/14837/Modules/_sqlite/cursor.c#newco...
Modules/_sqlite/cursor.c:705: if (!multiple && (statement_type ==
STATEMENT_INSERT || statement_type == STATEMENT_REPLACE)) {
Can we wrap this line?  (I'm not sure what the PEP7 standards for wrapping style
are, but I know it is <= 79 cols, just like everything else, even if a lot of
our existing code doesn't follow it :)
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7