Skip to content

bpo-45512: Simplify manage isolation level #29562

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

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Nov 15, 2021

@corona10
Copy link
Member Author

@erlend-aasland Can you please take a look?

@corona10
Copy link
Member Author


0:00:00 load avg: 1.52 Run tests sequentially
0:00:00 load avg: 1.52 [1/1] test_sqlite3
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 4.3 sec
Tests result: SUCCESS

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

I like this change! However, I think we can improve it further :) See my comments; let me know what you think.

@@ -199,14 +188,14 @@ pysqlite_connection_init_impl(pysqlite_Connection *self,
}

if (isolation_level) {
const char *stmt = get_begin_statement(isolation_level);
if (stmt == NULL) {
const char *level = get_isolation_level(isolation_level);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that there is no "isolation level to begin statement" conversion happening here, I find the get_isolation_level name a bit strange. I would prefer to rename get_isolation_level to validate_isolation_level and make it return 0 on success and -1 on error.

Another thing: it would be super nice to add a custom AC converter that could handle this conversion. That would mean that __init__ was given a completely valid const char *isolation_level, and we could just place it in self->isolation_level without any fuzz! It would make __init__ even simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

it would be super nice to add a custom AC converter that could handle this conversion.

I will left this part for you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on it ;) I can submit a PR for it after this has landed. I don't think I can modify your PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to rename get_isolation_level to validate_isolation_level and make it return 0 on success and -1 on error.

I withdraw this suggestion. We need to revert this particular change; we need to be sure that the pointer is valid as long as the connection object is alive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on it ;) I can submit a PR for it after this has landed. I don't think I can modify your PR.

FTR: I opened GH-29593

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM (with small nitpick comment; you can ignore it if you want). I'll add the AC converter in a new PR :)

@erlend-aasland
Copy link
Contributor

(See my DM on Discord before merging)

@corona10
Copy link
Member Author

@erlend-aasland

>>> import sqlite3
>>> cx = sqlite3.connect(":memory:", isolation_level="DEFERRED")
>>> cx.isolation_level
'DEFERRED'
>>> cx.isolation_level
'DEFERRED'
>>> cx.isolation_level = "immediate"
>>> cx.isolation_level
'IMMEDIATE'
>>>

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM. Let's pull in the tests before landing.

@erlend-aasland
Copy link
Contributor

FYI, you need to merge in main, now that GH-28227 has landed.

@corona10
Copy link
Member Author

@erlend-aasland
I check this PR with #29576 and it works as expected :)

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@corona10 corona10 merged commit e002bbc into python:main Nov 17, 2021
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
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.

4 participants