GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-83638: Add sqlite3.Connection.autocommit for PEP 249 compliant behaviour by erlend-aasland · Pull Request #93823 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jun 14, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Resolves #83638

TODO

  • Wait for PEP-249 amendments to land
  • Reflow and reindent doc changes
  • Issue: Add an autocommit property to sqlite3.Connection with a PEP 249 compliant manual commit mode and migrate #83638

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
erlend-aasland added 2 commits June 14, 2022 21:52
@erlend-aasland
Add sqlite3.Connection.autocommit for PEP 249 compliant behaviour
10d3a65
@erlend-aasland
Add basic tests
6ca9045
@bedevere-bot bedevere-bot added the awaiting core review label Jun 14, 2022
This was referenced Jun 14, 2022
Add an autocommit property to sqlite3.Connection with a PEP 249 compliant manual commit mode and migrate #83638
Closed
Add sqlite3.Connection.autocommit for PEP 249 compliant behaviour erlend-aasland/cpython#35
Closed
erlend-aasland added 3 commits June 14, 2022 22:54
@erlend-aasland
Add documentation
5772676
@erlend-aasland
Add NEWS
06609f3
@erlend-aasland
Add What's New
8b6b866
@erlend-aasland
erlend-aasland force-pushed the sqlite-autocommit branch from 7a613d3 to 8b6b866 Compare June 14, 2022 20:54
erlend-aasland added 3 commits June 14, 2022 23:00
@erlend-aasland
Document DEPRECATED_TRANSACTION_CONTROL
a604a57
@erlend-aasland
Fix setattr behaviour
cbc1041
@erlend-aasland
Suggest new behaviour starting with Python 3.14
41b898c
@erlend-aasland

erlend-aasland commented Jun 15, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland marked this pull request as ready for review June 15, 2022 08:05
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner June 15, 2022 08:05
@erlend-aasland
erlend-aasland requested review from malemburg and serhiy-storchaka June 15, 2022 08:06
@erlend-aasland

erlend-aasland commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

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

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jun 15, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood
AlexWaygood self-requested a review June 15, 2022 08:36
erlend-aasland
erlend-aasland commented Jun 15, 2022
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_transactions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

cc. @bitdancer

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Simplify implicit rollback test
b11ebd2
erlend-aasland
erlend-aasland commented Jun 15, 2022
View reviewed changes
Comment thread Modules/_sqlite/module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Jun 15, 2022
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address Alex' review
d3009d4
@erlend-aasland

erlend-aasland commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

Thanks, Alex! 🚀

AlexWaygood reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 3 commits June 15, 2022 23:09
@erlend-aasland
Reorder if condition to add emphasis to deprecated behaviour
fb98615
@erlend-aasland
Add context manager tests
5e4626e
@erlend-aasland
Improve 5e4626e
a8e3b9e
CAM-Gerlach
CAM-Gerlach reviewed Nov 9, 2022
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland and others added 2 commits November 11, 2022 14:14
Erlend E. Aasland @CAM-Gerlach
Update Doc/library/sqlite3.rst
4feefa1
Co-authored-by: C.A.M. Gerlach <[email protected]>
@erlend-aasland
Sync with main
720ac11
CAM-Gerlach
CAM-Gerlach reviewed Nov 11, 2022
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland @CAM-Gerlach
Update Doc/library/sqlite3.rst
f7c2ae4
Co-authored-by: C.A.M. Gerlach <[email protected]>
@geryogam

geryogam commented Nov 11, 2022

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 11, 2022

Copy link
Copy Markdown
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.

geryogam reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Sync with main
88e1420
@erlend-aasland erlend-aasland removed the DO-NOT-MERGE label Nov 12, 2022
geryogam
geryogam suggested changes Nov 12, 2022
View reviewed changes

@geryogam geryogam left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_sqlite3/test_transactions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_sqlite/module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@zzzeek zzzeek mentioned this pull request Nov 12, 2022
support sqlite .autocommit parameter (Python 3.12) sqlalchemy/sqlalchemy#8795
Open
@erlend-aasland
Address review: remove unneeded assert in test_autocommit_setget_invalid
7b87830
@erlend-aasland

erlend-aasland commented Nov 12, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

geryogam
geryogam suggested changes Nov 12, 2022
View reviewed changes

@geryogam geryogam left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_sqlite3/test_transactions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address review: add test for rollback() if autocommit is True
8322683
@erlend-aasland
erlend-aasland requested a review from geryogam November 12, 2022 20:34
@erlend-aasland

erlend-aasland commented Nov 12, 2022

Copy link
Copy Markdown
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!

geryogam reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

geryogam
geryogam suggested changes Nov 12, 2022
View reviewed changes

@geryogam geryogam left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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. =)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address review: fixup context manager docs
aca1e22
@erlend-aasland
erlend-aasland merged commit c95f554 into python:main Nov 12, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Nov 12, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-autocommit branch November 12, 2022 22:44
@geryogam

geryogam commented Nov 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

geryogam, erlend-aasland, and CAM-Gerlach reacted with hooray emoji
All reactions
  • 🎉 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@geryogam geryogam mentioned this pull request Nov 13, 2022
Improve sqlite3 documentation of the new autocommit attribute #99435
Closed
This was referenced Nov 22, 2022
Improve sqlite3 documentation of the new autocommit attribute #99670
Closed
Document sqlite3.connect() as implicitly opening transactions in the new PEP-249 manual commit mode #99824
Closed
Clarify sqlite3 documentation of PEP 249 compliance for commit modes #99828
Closed
@geryogam geryogam mentioned this pull request Dec 3, 2022
urllib.parse wrongly strips empty #fragment, ?query, //netloc #67041
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Aug 6, 2026
rel(infra-db): épingler le mode transactionnel — pas isolation_level, qui ne protège pas (#428) Thomas05000005/CineSort#976
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@malemburg malemburg Awaiting requested review from malemburg
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
+1 more reviewer
@geryogam geryogam geryogam requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @AlexWaygood @malemburg @CAM-Gerlach @geryogam @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.