GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-16379: expose SQLite error codes and error names in `sqlite3` by erlend-aasland · Pull Request #27786 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Aug 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
danielshahaf reacted with hooray emoji
All reactions
  • 🎉 1 reaction
palaviv and others added 29 commits May 9, 2019 12:54
@palaviv
sqlite3 module expose sqlite error code and name in exceptions
83ab267
@palaviv
Fix syntax error
eeb10f4
@palaviv
Add NEWS entry
42636f9
@palaviv
Update version
f224223
@palaviv
Fix whitespace
31467e3
Erlend E. Aasland
Merge branch 'main' into sqlite-expose-error-code
679280d
Erlend E. Aasland
Move exception documentation to the exceptions section
8aa0859
Erlend E. Aasland
Use PyModule_AddObjectRef to add error codes
e4195c0
Erlend E. Aasland
Refactor: add add_error_constants()
a4a0762
Erlend E. Aasland
Sort error codes alphabetically
c18d227
Erlend E. Aasland
Normalise style with the rest of the code base (PEP 7, etc.)
67e49e8
Erlend E. Aasland
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
Erlend E. Aasland
Refactor _pysqlite_seterror changes
67ea5f2 
- Use intermingled declarations
- Simplify ref count handling
Erlend E. Aasland
Refactor error code table
6ced38c 
- Declutter add_error_constants()
- No need to typedef struct
- Simplify naming
- Use PyModule_AddIntConstant
Erlend E. Aasland
Use macro to create error table
467f7ba 
To avoid mismatch between char *name and constant
Erlend E. Aasland
Add missing SQLITE_NOTICE and SQLITE_WARNING codes
4abd093
Erlend E. Aasland
Add comment
70507dd
Erlend E. Aasland
Merge branch 'main' into sqlite-expose-error-code
b617192
Erlend E. Aasland
Assert exception class is always set
db053b9
Erlend E. Aasland
Use explicit API's iso. Py_BuildValue, etc.
98106dd
Erlend E. Aasland
Remove duplicate return code entries
0567938
Erlend E. Aasland
Group stuff
16889a1
Erlend E. Aasland
Use 'unknown' when a proper SQLite exception cannot be found
a7a1cf8 
IMO, we should not pollute the SQLITE_* namespace
Erlend E. Aasland
Fix complete_statement.py example
772a29c 
- Use correct attribute name
- Normalise error variable naming
- Remove 'Errno' from printed string to avoid confusion
Erlend E. Aasland
Improve unit tests
fa5a1af 
- Use assertRaisesRegex iso. two assert functions
- Normalise quotes
- Test that constants are added to the module
Erlend E. Aasland
Adjust NEWS entry and update What's New
cf551b1
Erlend E. Aasland
Use PyObject_SetAttrString iso. _PyObject_SetAttrId => slow path/read…
a3646c9 
…ability
Erlend E. Aasland
Refactor _pysqlite_seterror
8a49659
Erlend E. Aasland
Link to sqlite.org/rescode.html iso. sqlite.org/c3ref/c_abort.html
8d11bc5
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner August 16, 2021 18:48
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 16, 2021
@bedevere-bot bedevere-bot added the awaiting review label Aug 16, 2021
@erlend-aasland

erlend-aasland commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

cc. @nedbat
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

cc. @auvipy & @matrixise who reviewed the original PR.
auvipy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland requested a review from pablogsal August 16, 2021 19:15
auvipy
auvipy approved these changes Aug 18, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Aug 18, 2021
Erlend E. Aasland
Use test.support.os_helper.temp_dir() in test_error_code_on_exception()
7a69798
@erlend-aasland

erlend-aasland commented Aug 19, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
auvipy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shihai1991
shihai1991 approved these changes Aug 22, 2021
View reviewed changes
pablogsal
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 pablogsal Aug 25, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@erlend-aasland erlend-aasland Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland Aug 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is it clearer with 3589a6a?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Erlend E. Aasland added 2 commits August 25, 2021 12:58
Erlend E. Aasland
Merge branch 'main' into sqlite-expose-error-code
b9a5a77
Erlend E. Aasland
Address review: document get_exception_class() return value
3589a6a
@erlend-aasland

erlend-aasland commented Aug 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Merge branch 'main' into sqlite-expose-error-code
a3c45b6
Spacetown
Spacetown reviewed Aug 26, 2021
View reviewed changes
Comment thread Modules/_sqlite/module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Spacetown
Spacetown reviewed Aug 26, 2021
View reviewed changes
Comment thread Modules/_sqlite/module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Address review: compare char ptr with NULL iso. 0
5a5683c
pablogsal
pablogsal approved these changes Aug 30, 2021
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Good work!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 30, 2021
@pablogsal
pablogsal merged commit 86d8b46 into python:main Aug 30, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 30, 2021
@erlend-aasland
erlend-aasland deleted the sqlite-expose-error-code branch August 30, 2021 19:04
@erlend-aasland

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland mentioned this pull request Aug 30, 2021
bpo-16379: Expose sqlite error code #1108
Closed
@erlend-aasland erlend-aasland mentioned this pull request Oct 7, 2021
bpo-16379: Fix SQLite version checks in test_module_constants() #28809
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@erlend-aasland @auvipy @shihai1991 @pablogsal @Spacetown @the-knights-who-say-ni @bedevere-bot @palaviv
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.