You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While working on bpo-45126 / #72414 (and while working on the AC conversion, bpo-40956), I've been slightly frustrated on the implementation of sqlite3 "isolation level". The code is messy, and we've got two connection members that carry pretty much the same type of information (self->isolation_level, and self->begin_statement).
I would like to make the following enhancements:
merge 'isolation_level' and 'begin_statement' members in some kind of way
split pysqlite_connection_set_isolation_level():
+ one method for parsing and storing the 'isolation_level' member
+ one method for carrying out any needed SQLite API operation
This should result in a cleaner connection __init__() method.
Another slightly related performance enhancement could be to cache the "begin" (and "commit") statements as sqlite3_stmt pointers on the connection object, but that is a digression.
Hmm, now I understood what you intended,
The code was hard to read without knowing the condition of the begin_statement.
Would this be a trade-off in terms of code readability?
IMO, #73239 improves readability, mainly because of these factors:
Argument Clinic now takes care of the ref count dance, and as a bonus, it is more self-documenting (stating clearly that it accepts str and None)
Begin statement validation has been refactored out, which IMO helps understanding how isolation level and begin statement is related
The isolation level setter is now greatly simplified; it fits well in a screenful, there are fewer ref count dances
Using get_begin_statement() is much less noisier than the previous string compare code in pysqlite_connection_set_isolation_level()
The only thing that still bugs me is the "magical" 6 byte offset needed to convert between isolation level and begin statement. Perhaps a set of conversion functions would make it more readable.
New changeset 822c3dc by Erlend Egeberg Aasland in branch 'main': bpo-45512: Raise exception if sqlite3.Connection.__init__ is called with bad isolation level (bpo-29561) 822c3dc
Activity
erlend-aasland commentedon Oct 18, 2021
While working on bpo-45126 / #72414 (and while working on the AC conversion, bpo-40956), I've been slightly frustrated on the implementation of sqlite3 "isolation level". The code is messy, and we've got two connection members that carry pretty much the same type of information (self->isolation_level, and self->begin_statement).
I would like to make the following enhancements:
+ one method for parsing and storing the 'isolation_level' member
+ one method for carrying out any needed SQLite API operation
This should result in a cleaner connection __init__() method.
Another slightly related performance enhancement could be to cache the "begin" (and "commit") statements as sqlite3_stmt pointers on the connection object, but that is a digression.
corona10 commentedon Nov 5, 2021
Hmm, now I understood what you intended,
The code was hard to read without knowing the condition of the begin_statement.
Would this be a trade-off in terms of code readability?
erlend-aasland commentedon Nov 8, 2021
IMO, #73239 improves readability, mainly because of these factors:
The only thing that still bugs me is the "magical" 6 byte offset needed to convert between isolation level and begin statement. Perhaps a set of conversion functions would make it more readable.
corona10 commentedon Nov 15, 2021
New changeset b567b9d by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Simplify isolation_level handling in
sqlite3(GH-29053)b567b9d
erlend-aasland commentedon Nov 15, 2021
Need to amend #73239, so I'm opening until the fix is merged.
corona10 commentedon Nov 15, 2021
New changeset 822c3dc by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Raise exception if sqlite3.Connection.__init__ is called with bad isolation level (bpo-29561)
822c3dc
corona10 commentedon Nov 17, 2021
New changeset 5f9247e by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Extend
sqlite3test suite regarding isolation levels (GH-29576)5f9247e
corona10 commentedon Nov 17, 2021
@erlendaasland
Now we close this issue right?
corona10 commentedon Nov 17, 2021
New changeset e002bbc by Dong-hee Na in branch 'main':
bpo-45512: Simplify manage isolation level (GH-29562)
e002bbc
erlend-aasland commentedon Nov 17, 2021
Not yet ;) I have an AC tweak I'd like to add. Wait for a PR anytime soon.
corona10 commentedon Nov 18, 2021
New changeset 0920b61 by Erlend Egeberg Aasland in branch 'main':
bpo-45512: Use Argument Clinic to set sqlite3 isolation level (GH-29593)
0920b61
erlend-aasland commentedon Nov 18, 2021
Thanks for reviews and improvements, Dong-hee :)