bpo-32788: Better error handling in sqlite3. by serhiy-storchaka · Pull Request #3723 · python/cpython · GitHub
Skip to content

bpo-32788: Better error handling in sqlite3. - #3723

Merged
serhiy-storchaka merged 13 commits into
python:masterfrom
serhiy-storchaka:sqlite-no-hasattr
Dec 10, 2018
Merged

bpo-32788: Better error handling in sqlite3.#3723
serhiy-storchaka merged 13 commits into
python:masterfrom
serhiy-storchaka:sqlite-no-hasattr

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Sep 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka added skip news type-bug An unexpected behavior, bug, or error labels Sep 24, 2017
@serhiy-storchaka serhiy-storchaka changed the title bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3. bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3. Sep 24, 2017
@serhiy-storchaka serhiy-storchaka changed the title bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3. bpo-32788: Better error handling in sqlite3. Feb 7, 2018
berkerpeksag
berkerpeksag reviewed Jul 23, 2018
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an old computer with no SQLite installed at the moment, but I think most of the patch looks good and safe to me. I have a question and a couple of comments:

  • Did you notice these by reading the code or noticed while using the module? For example, pysqlite_cache_get() (although it's exposed the cache implementation is an implementation detail and basically useless for third-party uses: https://bugs.python.org/issue30262) is only used by _pysqlite_query_execute() and it only accepts a string object. The only exception that may be raised here is OOM.

  • I'm fine with making old code PEP 7 compliant if you've already change the signature of the function, but in cases like below, I think it's better to keep the old code as-is:

    -static int check_cursor(pysqlite_Cursor* cur)
+static int
+check_cursor(pysqlite_Cursor* cur)

    The rest is fine because you've added static to them.

  • I haven't look at the code in microprotocols.c recently, so I still need a bit more time to review it and the code relevant to it (e.g. statement.c)

@serhiy-storchaka

serhiy-storchaka commented Jul 23, 2018

Copy link
Copy Markdown
Member Author

@berkerpeksag

berkerpeksag commented Jul 23, 2018

Copy link
Copy Markdown
Member

Do you mean just this function or all cases in which the only change is inserting a newline before the function name?

I missed that pysqlite_cursor_init() and pysqlite_cursor_dealloc() were already have static keyword. So, let's revert back to the old style in the following functions:

  • check_cursor()
  • pysqlite_cursor_init()
  • pysqlite_cursor_dealloc()

I am wondering if it is worth to document and test some user-visible behavior changes.

I think that would be great if you have time to do so :)

@serhiy-storchaka

serhiy-storchaka commented Dec 4, 2018

Copy link
Copy Markdown
Member Author

@berkerpeksag, have I addressed all your comments? Could you please take a look at this PR again?

@serhiy-storchaka

serhiy-storchaka commented Dec 10, 2018

Copy link
Copy Markdown
Member Author

I am going to merge this PR.

berkerpeksag
berkerpeksag approved these changes Dec 10, 2018
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I'd like to deprecate and eventually get rid of this pre-PEP 246 protocol.

BTW, I'm moving to a new country in a month, so I don't have time to closely check my GitHub notifications. Feel free to send me an email if I'm blocking a pull request.

@serhiy-storchaka

serhiy-storchaka commented Dec 10, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka
serhiy-storchaka merged commit fc662ac into python:master Dec 10, 2018
@serhiy-storchaka
serhiy-storchaka deleted the sqlite-no-hasattr branch December 10, 2018 14:06
@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

Assignees

No one assigned

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@serhiy-storchaka @berkerpeksag @the-knights-who-say-ni @bedevere-bot