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.

Sign up for GitHub

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

Jump to bottom

gh-61162: Clarify sqlite3 connection context manager docs #93890

Merged
merged 16 commits into from
Jun 19, 2022
Merged

gh-61162: Clarify sqlite3 connection context manager docs #93890

merged 16 commits into from
Jun 19, 2022

Conversation

erlend-aasland
Copy link
Contributor

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

Explicitly note that transactions are only closed if there is an open
transation at __exit__, and that transactions are not implicitly
opened during __enter__.

Co-authored-by: CAM Gerlach CAM.Gerlach@Gerlach.CAM
Co-authored-by: Stanley 46876382+slateny@users.noreply.github.com

Automerge-Triggered-By: GH:erlend-aasland

@erlend-aasland
pythongh-61162: Clarify sqlite3 connection context manager docs
672ce97 
Explicitly note that transactions are only closed if there is an open
transation, and that transactions are _not_ implicitly opened in
context manager __enter__.
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting core review labels Jun 16, 2022
@erlend-aasland erlend-aasland mentioned this pull request Jun 16, 2022
Closed
@erlend-aasland erlend-aasland requested a review from bitdancer June 16, 2022 08:12
@erlend-aasland
Copy link
Contributor Author

cc. @python/proofreaders

@Kodiologist, would you like to review this?

@erlend-aasland erlend-aasland added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jun 16, 2022
@AA-Turner
Copy link
Member

I can't do an inline review as lines have been deleted and added. I'd suggest though focussing on the happy path of an open transaction and a successful body first, i.e.:

With an open transaction, connection objects can be used as context managers
that automatically commit or rollback transactions. If the body of the ``with``
statement finishes without exceptions, the transaction is committed. If an
exception is raised and not caught, the transaction is rolled back.

If there is no open transaction, the context manager is a no-op.

A

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 16, 2022 •
edited
Loading

I can't do an inline review as lines have been deleted and added. I'd suggest though focussing on the happy path of an open transaction and a successful body first, i.e.:

Thanks, that's an improvement! PTAL.

AA-Turner
AA-Turner approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@AA-Turner AA-Turner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

✅

A

@Kodiologist
Copy link
Contributor

@Kodiologist, would you like to review this?

I'm not a real CPython maintainer, but this looks good to me. Thanks.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 16, 2022 •
edited
Loading

@Kodiologist, would you like to review this?

I'm not a real CPython maintainer, but this looks good to me. Thanks.

You don't need to be; "external" reviews are appreciated :) Thanks for reviewing!

Note: I pinged you since you commented recently on the issue.

@erlend-aasland
Copy link
Contributor Author

I'll let this PR sit around a day or two before merging.

mwichmann
mwichmann reviewed Jun 16, 2022
View reviewed changes
bskinn
bskinn reviewed Jun 16, 2022
View reviewed changes
CAM-Gerlach
CAM-Gerlach reviewed Jun 16, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Some Sphinx and writing suggestions

Erlend Egeberg Aasland and others added 4 commits June 17, 2022 01:18
@CAM-Gerlach
Add context manager anchor
28c435f 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach
Use anchor in NEWS entry
6baac81 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach
Update Doc/library/sqlite3.rst
8808fb9 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach
Add connection class link
7237399 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@erlend-aasland
Copy link
Contributor Author

I'll say it again, and see what happens: I'll let this PR sit around for a couple of days more before merging :)

Thanks, Proofreaders 🪄

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jun 17, 2022
@erlend-aasland
Incorporate pythongh-93890 changes
77651c0
@erlend-aasland erlend-aasland self-assigned this Jun 17, 2022
CAM-Gerlach
CAM-Gerlach approved these changes Jun 18, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @erlend-aasland !

@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 8e08978 into python:main Jun 19, 2022
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @erlend-aasland, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 8e0897814109765a9e463676413fff016875217b 3.11

@bedevere-bot
Copy link

GH-94012 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 19, 2022
@erlend-aasland erlend-aasland deleted the sqlite-ctx-mgr branch June 19, 2022 20:18
@erlend-aasland erlend-aasland added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Jun 19, 2022
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-94013 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 19, 2022
@erlend-aasland erlend-aasland mentioned this pull request Jul 27, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwichmann mwichmann mwichmann left review comments

@AA-Turner AA-Turner AA-Turner approved these changes

@bskinn bskinn bskinn left review comments

@slateny slateny slateny left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@ezio-melotti ezio-melotti ezio-melotti approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

@bitdancer bitdancer Awaiting requested review from bitdancer

Assignees

@miss-islington miss-islington

Labels
docs Documentation in the Doc dir
Projects
sqlite3 issues
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.