Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 34 commits into from Aug 30, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 16, 2021

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
- 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
- Use intermingled declarations
- Simplify ref count handling
- Declutter add_error_constants()
- No need to typedef struct
- Simplify naming
- Use PyModule_AddIntConstant
To avoid mismatch between char *name and constant
IMO, we should not pollute the SQLITE_* namespace
- Use correct attribute name
- Normalise error variable naming
- Remove 'Errno' from printed string to avoid confusion
- Use assertRaisesRegex iso. two assert functions
- Normalise quotes
- Test that constants are added to the module
@erlend-aasland
Copy link
Contributor Author

cc. @nedbat

@erlend-aasland
Copy link
Contributor Author

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

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 19, 2021

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

Thanks for reviewing, @auvipy

int
_pysqlite_seterror(pysqlite_state *state, sqlite3 *db)
static PyObject *
get_exception_class(pysqlite_state *state, int errorcode)
Copy link
Member

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

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)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 25, 2021

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

Modules/_sqlite/module.c Outdated Show resolved Hide resolved
Modules/_sqlite/module.c Outdated Show resolved Hide resolved
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM.

Good work!

@pablogsal pablogsal merged commit 86d8b46 into python:main Aug 30, 2021
@erlend-aasland erlend-aasland deleted the sqlite-expose-error-code branch August 30, 2021 19:04
@erlend-aasland
Copy link
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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants