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-94017: Improve clarity of sqlite3 transaction handling docs #94320

Merged
merged 11 commits into from
Jul 6, 2022
Merged

gh-94017: Improve clarity of sqlite3 transaction handling docs #94320

merged 11 commits into from
Jul 6, 2022

Conversation

erlend-aasland
Copy link
Contributor

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

Fixes gh-94017

@bedevere-bot bedevere-bot added awaiting core review docs Documentation in the Doc dir labels Jun 27, 2022
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 27, 2022 •
edited
Loading

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.

@AlexWaygood AlexWaygood self-requested a review June 27, 2022 09:01
CAM-Gerlach
CAM-Gerlach reviewed Jun 30, 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.

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

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/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
AlexWaygood
AlexWaygood reviewed Jul 1, 2022
View reviewed changes
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.

+1 to most of Cam's thoughts

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

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

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 1, 2022 •
edited
Loading

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.

@CAM-Gerlach
Copy link
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.

CAM-Gerlach
CAM-Gerlach approved these changes Jul 2, 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

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

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

erlend-aasland commented Jul 3, 2022 •
edited
Loading

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.

AlexWaygood
AlexWaygood reviewed Jul 3, 2022
View reviewed changes
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
@AlexWaygood
Typo
7aa11d5 
Co-authored-by: Alex Waygood <[email protected]>
@CAM-Gerlach
Copy link
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.

CAM-Gerlach
CAM-Gerlach reviewed Jul 6, 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.

Review feedback on the final revisisions

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
@erlend-aasland
Copy link
Contributor Author

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

@erlend-aasland erlend-aasland added needs backport to 3.10 only security fixes 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
Copy link
Contributor

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

@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
@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
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 6, 2022
@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 only security fixes label Jul 6, 2022
@bedevere-bot
Copy link

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

@erlend-aasland erlend-aasland mentioned this pull request Jul 6, 2022
Closed
miss-islington added a commit that referenced this pull request Jul 6, 2022
@miss-islington
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
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
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWaygood AlexWaygood AlexWaygood left review comments

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

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

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