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-96168: Add sqlite3 row factory how-to #99507

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Nov 15, 2022

Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
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 .

Overall, I found the examples helpful, and a good balance between concise and detailed. However, maybe this is just me, but to me the section did appear pretty heavy on examples, but without as much explicit focus on solving specific user problems, as would be appropriate for a how-to. In that sense, it almost feels more like a tutorial, having readers learn using row factories by example, as opposed to telling them how to solve specific problems using row factories.

@ezio-melotti , what do you think?

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 Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

However, maybe this is just me, but to me the section did appear pretty heavy on examples, but without as much explicit focus on solving specific user problems, as would be appropriate for a how-to.

Yeah, maybe... but How To's are recipes. How can I create a foo row factory? How do I use a bar row factory? How do I use the sqlite3.Row row factory? I don't think it matters much that it is example heavy. IMO it is too thin for a tutorial, and definitely too thin for an explanation.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Nov 16, 2022

Ouch, it just occurred to me that the Cursor also has a row_factory attribute, with the following problems:

  • it is not documented (easily fixed; we can do that in this PR done in this PR)
  • it is copied from the connection in Cursor.__init__, so changing the connection row factory does not affect the row factories of the cursors belonging to that connection; vice versa

So, more prose coming up.

>>> import sqlite3
>>> cx = sqlite3.connect(":memory:")
>>> cx.row_factory = sqlite3.Row
>>> cu = cx.cursor()
>>> cx.row_factory = None
>>> cu.row_factory == cx.row_factory
False
>>> cu.connection.row_factory == cx.row_factory
True

@erlend-aasland
Copy link
Contributor Author

@CAM-Gerlach: I've reworded the reference, hopefully to the better; IMO the current version reflects the difference between the two row factory attributes more clearly. I also removed the example from ab98877; IMO it is superfluous with the now (hopefully) improved reference. Looking forward to your comments.

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.

This is indeed looking pretty good, thanks. Follow-up suggestions are basically just textual tweaks.

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

Looks pretty good to me now, personally, Just a couple minor suggestions and followups to others' comments.

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 Outdated Show resolved Hide resolved
@merwok merwok removed their request for review November 24, 2022 06:42
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.

Just one comment cleaning up my own mess, otherwise LGTM from my side, thanks.

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

Thanks again y'all for improving this part of the docs. I'm going to land this later today. A lot of back and forth, but IMO totally worth it.

@erlend-aasland erlend-aasland merged commit 8749121 into python:main Nov 25, 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-docs/row-factory-how-to branch November 25, 2022 13:07
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2022
(cherry picked from commit 8749121)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@bedevere-bot
Copy link

GH-99778 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 Nov 25, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2022
(cherry picked from commit 8749121)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@bedevere-bot
Copy link

GH-99779 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 Nov 25, 2022
miss-islington added a commit that referenced this pull request Nov 25, 2022
(cherry picked from commit 8749121)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
miss-islington added a commit that referenced this pull request Nov 25, 2022
(cherry picked from commit 8749121)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants