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-94017: Improve clarity of sqlite3 transaction handling docs by erlend-aasland · Pull Request #94320 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixes gh-94017

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-94017: Improve clarity of sqlite3 transaction handling docs
91460c0
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner June 27, 2022 08:52
@bedevere-bot bedevere-bot added awaiting core review docs Documentation in the Doc dir labels Jun 27, 2022
@erlend-aasland erlend-aasland added the DO-NOT-MERGE label Jun 27, 2022
@erlend-aasland

erlend-aasland commented Jun 27, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

cc. @CAM-Gerlach, @AlexWaygood. Would you two mind reviewing this? I'll go over it later today, to be sure the docs align with the actual implementation. Other that, I worry that the new tone of the docs may come across as ... perhaps a trifle strict, or negative sounding (in lack of more appropriate words). I also worry about the added verbosity. I do hope this makes things clearer, though.

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 27, 2022 09:01
@erlend-aasland
Incorporate additional changes from pythongh-93823
a531aa8
erlend-aasland
erlend-aasland commented Jun 27, 2022
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

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

@AlexWaygood AlexWaygood added the skip news label Jun 27, 2022
erlend-aasland
erlend-aasland commented Jun 27, 2022
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

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

Erlend Egeberg Aasland and others added 4 commits June 28, 2022 01:25
Erlend Egeberg Aasland
Update Doc/library/sqlite3.rst
b09fab1
@erlend-aasland
Adjust wording
f9f4f97
@erlend-aasland
Merge remote-tracking branch 'origin/sqlite-doc-tx-handling' into sql…
80d29aa
…ite-doc-tx-handling
@erlend-aasland
Make it clearer that SQLite autocommit mode is a concept belonging to…
576c760
… the underlying library
CAM-Gerlach
CAM-Gerlach reviewed Jun 30, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

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.

Thanks @erlend-aasland , this is a definite improvement. I do have some clarity, textual and reST fixes and suggestions.

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.

AlexWaygood
AlexWaygood reviewed Jul 1, 2022
View reviewed changes

@AlexWaygood AlexWaygood left a comment

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.

+1 to most of Cam's thoughts

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.

@erlend-aasland

erlend-aasland commented Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

Thanks, both of you; highly appreciated. I'm finally getting around to address the reviews.

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
Address reviews
a530b3e
@erlend-aasland

erlend-aasland commented Jul 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Reviews addressed in a530b3e.
I mostly went for ~Connection.isolation_level style links instead of the more explicit Connection.isolation_level style links. I can use the latter if that's preferred.

All reactions

Sorry, something went wrong.

Uh oh!

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

@CAM-Gerlach

CAM-Gerlach commented Jul 2, 2022

Copy link
Copy Markdown
Member

I mostly went for ~Connection.isolation_level style links instead of the more explicit Connection.isolation_level style links. I can use the latter if that's preferred.

That's fine with me, since it isn't used on multiple classes in the module and links directly to them in either case. I wasn't very consistent about using that myself.

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.

CAM-Gerlach
CAM-Gerlach approved these changes Jul 2, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

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.

LGTM, thanks @erlend-aasland

I had one comment, but that might be out of scope here and better addressed in a future PR.

Sorry, something went wrong.

Uh oh!

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

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

Uh oh!

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

erlend-aasland
erlend-aasland commented Jul 2, 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
Last rewrite, I promise
1473e94
@erlend-aasland

erlend-aasland commented Jul 3, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I'm sorry, but I did a last rewrite of things, trying to address #94320 (comment). Hopefully to the better. I'd be happy if you could review commit 1473e94. If it was for the worse, I'll revert it.

For the "Controlling Transactions" section, I tried to improve the visual separation of the two sqlite3 transaction modes.

All reactions

Sorry, something went wrong.

Uh oh!

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

AlexWaygood
AlexWaygood reviewed Jul 3, 2022
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

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

Erlend Egeberg Aasland @AlexWaygood
Typo
7aa11d5
Co-authored-by: Alex Waygood <[email protected]>
@CAM-Gerlach

CAM-Gerlach commented Jul 5, 2022

Copy link
Copy Markdown
Member

I'm sorry, but I did a last rewrite of things, trying to address #94320 (comment). Hopefully to the better. I'd be happy if you could review commit 1473e94. If it was for the worse, I'll revert it.

No need to apologize for doing more work and making the docs better! I'll review it now—sorry myself for the delay; it was a holiday weekend and I was with family.

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

CAM-Gerlach
CAM-Gerlach reviewed Jul 6, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

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.

Review feedback on the final revisisions

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.

@erlend-aasland
Address most of CAM's review
7418668
Erlend Egeberg Aasland @CAM-Gerlach
Update Doc/library/sqlite3.rst
be859ae
Co-authored-by: CAM Gerlach <[email protected]>
@erlend-aasland

erlend-aasland commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

Thanks again for the reviews; I'll land this later tonight.

CAM-Gerlach 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 added needs backport to 3.10 needs backport to 3.11 only security fixes and removed DO-NOT-MERGE labels Jul 6, 2022
@erlend-aasland
erlend-aasland merged commit 760b8cf into python:main Jul 6, 2022
@miss-islington

miss-islington commented Jul 6, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting core review label Jul 6, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-doc-tx-handling branch July 6, 2022 20:59
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 6, 2022
Erlend Egeberg Aasland @miss-islington
pythongh-94017: Improve clarity of sqlite3 transaction handling docs (p…
dc3ff9d
…ythonGH-94320)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 760b8cf)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 6, 2022
@bedevere-bot

bedevere-bot commented Jul 6, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 6, 2022
Erlend Egeberg Aasland @miss-islington
pythongh-94017: Improve clarity of sqlite3 transaction handling docs (p…
667f4b8
…ythonGH-94320)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 760b8cf)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Jul 6, 2022
@bedevere-bot

bedevere-bot commented Jul 6, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland mentioned this pull request Jul 6, 2022
Improve clarity of sqlite3 transaction handling docs #94017
Closed
miss-islington added a commit that referenced this pull request Jul 6, 2022
@miss-islington Erlend Egeberg Aasland
gh-94017: Improve clarity of sqlite3 transaction handling docs (GH-94320
7e3b6af
)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 760b8cf)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
miss-islington added a commit that referenced this pull request Jul 6, 2022
@miss-islington Erlend Egeberg Aasland
gh-94017: Improve clarity of sqlite3 transaction handling docs (GH-94320
37a47b1
)

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 760b8cf)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request Jul 25, 2022
Document all parameters of all sqlite3 functions/methods #95235
Closed
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 left review comments
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

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

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve clarity of sqlite3 transaction handling docs

Uh oh!

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

5 participants

@erlend-aasland @CAM-Gerlach @miss-islington @bedevere-bot @AlexWaygood
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.