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

gh-83638: Add sqlite3.Connection.autocommit for PEP 249 compliant behaviour #93823

Merged
merged 82 commits into from
Nov 12, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 14, 2022

Resolves #83638

TODO

  • Wait for PEP-249 amendments to land
  • Reflow and reindent doc changes

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 15, 2022

@zzzeek, @maggyero, I'd appreciate your review on this, if only by building and testing it, and/or taking a look at the added tests.

@erlend-aasland erlend-aasland marked this pull request as ready for review June 15, 2022 08:05
@erlend-aasland
Copy link
Contributor Author

@AlexWaygood would you mind taking a look at the doc changes?

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 15, 2022

For external reviewers, here's some help on how to get started reviewing this PR:

  • Rationale / proposed solution
    • First, read up on the discussions in the linked issue, and those linked from that issue
    • Are there holes or inconsistencies in the proposed solution?
    • Does the proposed solution actually solve the problem?
  • Implementation
    • Does the implementation align with the proposed solution?
    • Does the implementation align with PEP 249?
    • Are there logical inconsistencies in the implementation?
  • Tests:
    • Do the added tests test the actual implementation?
    • Could the added tests be more specific?
    • Are there missing corner cases?
    • Are there superfluous tests?
    • How is the code coverage? Are there uncovered functions/lines/branches?
  • Docs:
    • Are the added docs helpful for the average user? (Too technical? Missing examples?)
    • Are the added docs aligned with the actual implementation
    • Are there spelling/syntax errors?
    • Is the NEWS entry understandable for the average user? Does it explain the change in a concise way?
    • Can the What's New entry be improved?
  • Backwards compatibility
    • Does the proposed solution follow PEP 387?
    • Could we add more emphasis on the deprecation notices?

@AlexWaygood AlexWaygood self-requested a review June 15, 2022 08:36
@erlend-aasland
Copy link
Contributor Author

cc. @bitdancer

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

A few small things, nothing major. (I only looked at the docs :)

Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Thanks, Alex! 🚀

@CAM-Gerlach
Copy link
Member

Deliberately not pushed; I'd made most of the changes, except for the last (until recently) unresolved conversation, as my proposed change was part of my local branch.

I had figured that, up until you then applied one of the suggestions to the remote branch, which combined with your statement made me a little unsure so I wanted to double-check (since that would mean your local unpushed changes and the remote branch would be both out of sync, in addition to applying GitHub suggestions directly being a little out of character for you 😂 )

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Nov 11, 2022

@maggyero: I am going to make a quick review today, so can you hold the merge a little bit more?

Thanks, I definitely welcome more eyes on this. The second 3.12 alpha is scheduled for today (2022-11-14); I'm aiming for alpha 3, which is scheduled for 2022-12-05. If you can make your review by the end of November, that would be nice.

Heads-up: you should take a look at the latest emails on the db-sig mailing list, so you have an up to date understanding of how the upcoming PEP 249 autocommit attribute will look.

EDIT: I mixed up the dates; since alpha 2 is scheduled for Monday, I'm going to aim for that with this PR. If you don't have time to review this now, @maggyero, feel free to do a post-merge review. If we need to change the implementation, we can do so all the way until the feature freeze in May (IIRC).

Erlend E. Aasland and others added 2 commits November 11, 2022 14:14
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@geryogam
Copy link
Contributor

EDIT: I mixed up the dates; since alpha 2 is scheduled for Monday, I'm going to aim for that with this PR. If you don't have time to review this now, @maggyero, feel free to do a post-merge review. If we need to change the implementation, we can do so all the way until the feature freeze in May (IIRC).

Thanks for the information and sorry for the delay, restoring the whole commit mode context took me longer than I expected. I actually did a complete rewriting of a post of mine on Stack Overflow on this topic to have a clear mental model.

Heads-up: you should take a look at the latest emails on the db-sig mailing list, so you have an up to date understanding of how the upcoming PEP 249 autocommit attribute will look.

Yes, I did.

@erlend-aasland
Copy link
Contributor Author

Thanks for the information and sorry for the delay, restoring the whole commit mode context took me longer than I expected. I actually did a complete rewriting of a post of mine on Stack Overflow on this topic to have a clear mental model.

No problem. I'll merge this tomorrow evening (CET) so I'm sure we get this into the next alpha. If you manage to do a review by then; good. If not, I'd be happy with a post-merge review, if you are fine with that.

Copy link
Contributor

@geryogam geryogam left a comment

Choose a reason for hiding this comment

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

This PR is great! Here is my review of the implementation, which is the most urgent. I’ll try to review the documentation this afternoon before the evening merge.

Lib/test/test_sqlite3/test_transactions.py Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
Modules/_sqlite/module.c Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

To everyone involved here and on the issue(s), thanks for fruitful discussions and helpful reviews. I'll handle the eventual deprecation of ´isolation_mode´ in a follow-up issue/PR; I'll close gh-83638 with this PR.

Copy link
Contributor

@geryogam geryogam left a comment

Choose a reason for hiding this comment

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

I added an extra change request in the tests.

Lib/test/test_sqlite3/test_transactions.py Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

@maggyero, I'm landing this now. If you have further remarks, comment on the issue and we'll do a follow-up PR. Thanks for the review!

Copy link
Contributor

@geryogam geryogam left a comment

Choose a reason for hiding this comment

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

And here is the documentation review. =)

Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
Doc/library/sqlite3.rst Show resolved Hide resolved
@erlend-aasland erlend-aasland merged commit c95f554 into python:main Nov 12, 2022
@erlend-aasland erlend-aasland deleted the sqlite-autocommit branch November 12, 2022 22:44
@geryogam
Copy link
Contributor

geryogam commented Nov 13, 2022

Thank you very much for your work and patience @erlend-aasland and the reviewers, I think you did an amazing job! This will improve the user experience significantly. According to the SQLite engine documentation, SQLite is the most widely deployed database engine and might even be the single most widely deployed software component of any type, so the SQLite3 driver for Python has to be of top quality.

I am fully convinced by the design (thanks @malemburg), implementation and tests. The documentation looks good to me, and we could address the remaining suggested changes in a follow-up documentation PR.

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.

Add an autocommit property to sqlite3.Connection with a PEP 249 compliant manual commit mode and migrate
6 participants