Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 23, 2022

Also add adapters and converter recipes.

Co-authored-by: CAM Gerlach [email protected]
Co-authored-by: Alex Waygood <[email protected]

Automerge-Triggered-By: GH:erlend-aasland

@erlend-aasland
Copy link
Contributor Author

Step one of gh-90016: reword adapter/converter section and add recipes. The latter point is very much a work in progress; I will incorporate some of Serhiy's suggestions here before marking this as ready for review.

We will backport this to 3.11 and 3.10, and then deprecate the default adapters/converters (in main only).

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 23, 2022

I also took the liberty of normalising the argument spec of register_adapter and register_converter; they are positional only, so the naming is irrelevant to the functionality of these methods. However, I would like to use this opportunity to normalise the docstrings.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 23, 2022

For new reviewers: suggestions to how to help reviewing this PR:

  • rewording: improve my English; I'm not a native speaker
  • userfriendlyness: is the rewritten docs more userfriendly or less userfriendly? why? how can it be improved?
  • examples: do the examples make sense? are they easily understood? how can they be improved?
  • brevity/verboseness: is this part of the docs too verbose? or the opposite? are there redundant passages (I said the same thing twice using different wordings)?
  • are there obvious reST markup issues?

@erlend-aasland erlend-aasland added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels May 23, 2022
- revert most argument spec changes
- align argument spec across docstrings/docs
- improve wording of register_adapter/register_converter
- undo note
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.

A fair few comments ;)

@erlend-aasland
Copy link
Contributor Author

A fair few comments ;)

Fantastic, thank you so much :) I've addressed your review in 0b6f381. I've left two of the conversations unresolved.

Now, onto the recipes.

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.

A few more tiny nits :)

@erlend-aasland
Copy link
Contributor Author

A few more tiny nits :)

Thanks again! Addressed in 72013ef

@erlend-aasland erlend-aasland changed the title [WIP] gh-90016: Reword sqlite3 adapter/converter docs gh-90016: Reword sqlite3 adapter/converter docs May 25, 2022
@erlend-aasland
Copy link
Contributor Author

I'm marking this as ready for review, although I expect more work on the recipes before this can land.

@malemburg, would you mind taking a look at this PR?

@erlend-aasland
Copy link
Contributor Author

Thank you so much for reviewing, everyone! I believe I've addressed everything1. I believe there is still room for improvement on this particular area, but this PR is already long and unwieldy.

Here are some quick'n dirty notes I made while reading this through for the umpteenth time:

  • The doc section for sqlite3.connect is very long and messy... it mentions type detection and SQLite types in probably three places (!), IIRC
  • The examples, guides, and tutorial'ish section (for adapters/converters) seem to repeat each other all the time... could benefit from consolidation... diataxis can perhaps help here. Perhaps it is time to create Doc/howto/sqlite3.rst?
  • The SQLite types (ref sqlite3-types) section could be made more to the point

Anyways, I'll leave this open for a few more days before merging, in case y'all want to review the recent updates yet again :)

Footnotes

  1. Well, almost everything. Some concerns will be addressed in future PRs, like for example documenting the obscure PrepareProtocol class.

@CAM-Gerlach
Copy link
Member

The examples, guides, and tutorial'ish section (for adapters/converters) seem to repeat each other all the time... could benefit from consolidation... diataxis can perhaps help here. Perhaps it is time to create Doc/howto/sqlite3.rst?

Yeah—the split itself should be fairly clean and straightforward since the high-level organization is already along these lines, though there's plenty of internal improvement to be done in helping address the other issues mentioned. Of course, this would be a followup PR.

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 now, at least so far as a reasonable scope of the changes go. Thanks @erlend-aasland ; this is a big improvement!

@CAM-Gerlach
Copy link
Member

The doc section for sqlite3.connect is very long and messy... it mentions type detection and SQLite types in probably three places (!), IIRC

Out of scope for this immediate PR, but here's a peak at a draft of how it could look rewritten as Reference documentation following Diataxis guidelines, using a consistent, structured format (Sphinxdoc) to be clearer, more concise/less repetitive, easier to navigate, and more complete (including param types, default values, etc), as discussed on Discourse.

Its around 1/3 shorter (despite including more information), and much easier to quickly scan and efficiently retrieve the specific information a user is looking for (what a param does, its type, default values, etc), without having to wade through paragraph after paragraph of narrative text, infer key details from implicit information, and jump through a chain of links to find something as simple as a default value.

Preview screenshot (click to expand)

image

@erlend-aasland
Copy link
Contributor Author

(FYI, I'll reflow the changes right before merging.)

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Jun 24, 2022

@erlend-aasland Here's the reST source from the draft of the reorganization above, if you'd prefer to use that as a base for a followup PR and iterate further on it yourself, as opposed to me submiting that as a PR on its own.

Source code (click to expand)
.. function:: connect(database, timeout=5.0, detect_types=0, isolation_level="DEFERRED", check_same_thread=True, factory=sqlite3.Connection, cached_statements=128, uri=False)

   Open a connection to a SQLite database file.

   :param database: The path to the database file to be opened.
       Pass ``":memory:"`` to open a connection to a database that is
       in RAM instead of on disk.
   :type database: :term:`path-like object`

   :param timeout: How many seconds the connection should wait before raising
       an exception, if the database is locked by another connection.
       If another connection opens a transaction to modify the database,
       it will be locked until that transaction is committed.
       Default five seconds.
   :type timeout: float

   :param detect_types: Whether/how data types not natively supported by SQLite
       are looked up to be converted to Python types,
       using the converters registered with :func:`register_converter`.
       Set it to any combination (using ``|``, bitwise or) of
       :const:`PARSE_DECLTYPES` and :const:`PARSE_COLNAMES`
       to enable this.
       Column names takes precedence over declared types if both flags are set.
       Types cannot be detected for generated fields (for example ``max(data)``),
       even when the *detect_types* parameter is set; :class:`str` will be
       returned instead.
       By default (``0``), type detection is disabled.
   :type detect_types: int

   :param isolation_level: The :attr:`~Connection.isolation_level` of the
       connection, controlling when ``BEGIN`` statements are implicitly executed.
       Can be ``"DEFERRED"`` (default), ``"EXCLUSIVE"`` or ``"IMMEDIATE"``;
       or None to enable autocommit;
       see :ref:`sqlite3-controlling-transactions` for more.
   :type isolation_level: str | None

   :param check_same_thread: If :const:`True` (default),
       only the creating thread may use the connection.
       If :const:`False`, the connection may be shared across multiple threads;
       if so, write operations
       should be serialized by the user to avoid data corruption.
   :type check_same_thread: bool

   :param factory: A custom subclass of :class:`Connection` to create the
       connection with, if not the default :class:`Connection` class.
   :type factory: :class:`Connection`

   :param cached_statements: The number of statements that :mod:`sqlite3`
       should internally cache for this connection, to avoid parsing overhead.
       By default, 128 statements.
   :type cached_statements: int

   :param uri: If set to :const:`True`, *database* is interpreted as a
       :abbr:`URI (Uniform Resource Identifier)` with a file path
       and an optional query string.
       The scheme part *must* be ``"file:"``,
       and the path can be relative or absolute.
       The query string allows passing parameters to SQLite.
       More information about this feature, including a list of parameters,
       can be found in the
       `SQLite URI documentation <https://www.sqlite.org/uri.html>`_.
   :type uri: bool

   :rtype: typing.Any

   .. audit-event:: sqlite3.connect database sqlite3.connect
   .. audit-event:: sqlite3.connect/handle connection_handle sqlite3.connect

   .. versionadded:: 3.4
      The *uri* parameter.

   .. versionchanged:: 3.7
      *database* can now also be a :term:`path-like object`, not only a string.

   .. versionadded:: 3.10
      The ``sqlite3.connect/handle`` auditing event.

Sidenote: my intention was to move the "trips and tricks with uri" example block to the "Guide" section/document, to avoid belaboring the API reference too much; otherwise, it contains all the same key information as the original + your changes here.

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland Here's the reST source from the draft of the reorganization above, if you'd prefer to use that as a base for a followup PR and iterate further on it yourself, as opposed to me submiting that as a PR on its own.

Thanks a lot!

@erlend-aasland erlend-aasland merged commit bd3c1c1 into python:main Jun 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-doc-converters branch June 25, 2022 20:06
@miss-islington
Copy link
Contributor

Sorry, @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker bd3c1c187e0e4fde5aec6835d180e9eddde8ceb6 3.11

@miss-islington
Copy link
Contributor

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 25, 2022
@bedevere-bot
Copy link

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

@bedevere-bot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants