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-42064: Move sqlite3 exceptions to global state, part 1 of 2 #26745

Merged
merged 9 commits into from
Jun 23, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 15, 2021

Also adds a test to verify the (borrowed) exceptions in sqlite3.Connection.

https://bugs.python.org/issue42064

Automerge-Triggered-By: GH:encukou

@erlend-aasland
Copy link
Contributor Author

@encukou Whenever you have time, would you mind reviewing this as well? I've split this operation in two; moving all exceptions was too large a diff.

@erlend-aasland erlend-aasland changed the title bpo-42064: Move sqlite3 exceptions to global state, part 1 bpo-42064: Move sqlite3 exceptions to global state, part 1 of 2 Jun 15, 2021
@encukou
Copy link
Member

encukou commented Jun 16, 2021

Looks OK at first glance but I'll do a review next week (after 3.10 beta 3)

@erlend-aasland
Copy link
Contributor Author

I just remembered that all the connection object keeps borrowed references to all the sqlite3 exception types; we should use that instead of the module state where possible. See 4fe9d98.

@erlend-aasland erlend-aasland requested a review from encukou June 22, 2021 22:33
@encukou
Copy link
Member

encukou commented Jun 23, 2021

I just remembered that all the connection object keeps borrowed references to all the sqlite3 exception types

I see the connection type is created with PyType_FromModuleAndSpec, so connections hold a reference to the module through their type. (So the module is kept alive and the connections can borrow from it safely).
But, borrowed references tend to be confusing – you always have to think about things like this, even in the future as changes made in seemingly unrelated parts.
Is it worth it to update them to strong references? Or just one strong reference to the module?

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

The changes themselves look OK!

@erlend-aasland
Copy link
Contributor Author

But, borrowed references tend to be confusing – you always have to think about things like this, even in the future as changes made in seemingly unrelated parts.
Is it worth it to update them to strong references? Or just one strong reference to the module?

Yes, I've thought about this. I think updating them to strong references is a good idea.

@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing, Petr!

@erlend-aasland erlend-aasland deleted the sqlite-move-err/1 branch June 23, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants