FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-16379: expose SQLite error codes and error names in `sqlite3` by erlend-aasland · Pull Request #27786 · python/cpython · GitHub

/ cpython Public

bpo-16379: expose SQLite error codes and error names in sqlite3 - #27786

Merged
pablogsal merged 34 commits into
python:mainfrom
erlend-aasland:sqlite-expose-error-code
Aug 30, 2021
ConversationCommits (34)ChecksFiles changed
Merged

bpo-16379: expose SQLite error codes and error names in sqlite3#27786
pablogsal merged 34 commits into
python:mainfrom
erlend-aasland:sqlite-expose-error-code

Conversation

erlend-aasland commented Aug 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Original PR: GH-1108 (contributed by @palaviv, based on patches by @danielshahaf)

This PR is a rebase and rework of Aviv & Daniel's work.

Co-authored-by: Aviv Pavlivoda
Co-authored-by: Daniel Shahaf
Co-authored-by: Erlend E. Aasland

https://bugs.python.org/issue16379

palaviv and others added 29 commits May 9, 2019 12:54
sqlite3 module expose sqlite error code and name in exceptions
83ab267
Fix syntax error
eeb10f4
Add NEWS entry
42636f9
Update version
f224223
Fix whitespace
31467e3
Merge branch 'main' into sqlite-expose-error-code
679280d
Move exception documentation to the exceptions section
8aa0859
Use PyModule_AddObjectRef to add error codes
e4195c0
Refactor: add add_error_constants()
a4a0762
Sort error codes alphabetically
c18d227
Normalise style with the rest of the code base (PEP 7, etc.)
67e49e8
Improve "get error name" function
1893ced 
- Naming: sqlite3ErrName => pysqlite_error_name
- Error handling:
    * return NULL if no exception matched
    * receiver handles errors
    * don't use error table to store SQLITE_UNKNOWN string
Refactor _pysqlite_seterror changes
67ea5f2 
- Use intermingled declarations
- Simplify ref count handling
Refactor error code table
6ced38c 
- Declutter add_error_constants()
- No need to typedef struct
- Simplify naming
- Use PyModule_AddIntConstant
Use macro to create error table
467f7ba 
To avoid mismatch between char *name and constant
Add missing SQLITE_NOTICE and SQLITE_WARNING codes
4abd093
Add comment
70507dd
Merge branch 'main' into sqlite-expose-error-code
b617192
Assert exception class is always set
db053b9
Use explicit API's iso. Py_BuildValue, etc.
98106dd
Remove duplicate return code entries
0567938
Group stuff
16889a1
Use 'unknown' when a proper SQLite exception cannot be found
a7a1cf8 
IMO, we should not pollute the SQLITE_* namespace
Fix complete_statement.py example
772a29c 
- Use correct attribute name
- Normalise error variable naming
- Remove 'Errno' from printed string to avoid confusion
Improve unit tests
fa5a1af 
- Use assertRaisesRegex iso. two assert functions
- Normalise quotes
- Test that constants are added to the module
Adjust NEWS entry and update What's New
cf551b1
Use PyObject_SetAttrString iso. _PyObject_SetAttrId => slow path/read…
a3646c9 
…ability
Refactor _pysqlite_seterror
8a49659
Link to sqlite.org/rescode.html iso. sqlite.org/c3ref/c_abort.html
8d11bc5

erlend-aasland commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

cc. @nedbat

erlend-aasland commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

cc. @auvipy & @matrixise who reviewed the original PR.

erlend-aasland commented Aug 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

FYI, I pushed a small tweak to the unit test: use the temp_dir helper iso. a hard coded path.

Thanks for reviewing, @auvipy

pablogsal reviewed Aug 25, 2021
View reviewed changes
Comment thread Modules/_sqlite/util.c
int
_pysqlite_seterror(pysqlite_state *state, sqlite3 *db)
static PyObject *
get_exception_class(pysqlite_state *state, int errorcode)

pablogsal Aug 25, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is slighly confusing because normally returning NULL signifies an error. Could you please document this both on the call site and in this function? Otherwise the call site reads weirdly because

    PyObject *exc_class = get_exception_class(state, errorcode);
    if (exc_class == NULL) {
        return errorcode;
    }

seems that is handling an error

erlend-aasland Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, I see now that it can be a bit confusing. I'll add a comment. Thanks!

erlend-aasland Aug 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it clearer with 3589a6a?

(FYI, I need to rebase onto main again bco. GH-26202)

erlend-aasland commented Aug 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'll sync with main after #27942 is merged. => synced with a3c45b6

Spacetown reviewed Aug 26, 2021
View reviewed changes
Comment thread Modules/_sqlite/module.c Outdated
Spacetown reviewed Aug 26, 2021
View reviewed changes
Comment thread Modules/_sqlite/module.c Outdated
pablogsal approved these changes Aug 30, 2021
View reviewed changes

pablogsal 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Good work!

pablogsal merged commit 86d8b46 into python:main Aug 30, 2021
erlend-aasland deleted the sqlite-expose-error-code branch August 30, 2021 19:04

erlend-aasland commented Aug 30, 2021

Copy link
Copy Markdown
Contributor Author

Thanks Pablo, and you can thank @danielshahaf & @palaviv for the good work; I just "stole" it, rebased onto main and made some minor adjustments :)

erlend-aasland mentioned this pull request Aug 30, 2021
Closed
erlend-aasland mentioned this pull request Oct 7, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

shihai1991 shihai1991 approved these changes
pablogsal pablogsal approved these changes
berkerpeksag Awaiting requested review from berkerpeksag
+2 more reviewers
Spacetown Spacetown left review comments
auvipy auvipy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL