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

Docs: use parameter list for sqlite3.Cursor.execute* #101782

Merged
merged 5 commits into from
Feb 10, 2023
Merged

Docs: use parameter list for sqlite3.Cursor.execute* #101782

merged 5 commits into from
Feb 10, 2023

Conversation

erlend-aasland
Copy link
Contributor

No description provided.

@bedevere-bot bedevere-bot added awaiting core review docs Documentation in the Doc dir labels Feb 10, 2023
@erlend-aasland
Copy link
Contributor Author

I'm not sure if this is an improvement; I need new eyes to tell me :) Also, I'm in doubt of how to use the :raises Sphinx param here; execute*() can raise a heck of a lot of exceptions; should we write all of them down here, or just a subset? Perhaps just point to the reference for sqlite3 exceptions?

AlexWaygood
AlexWaygood reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

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

I think this is a nice improvement! I find it much easier to understand when parameters should be a sequence and when it should be a dict with this change

Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
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
erlend-aasland and others added 2 commits February 10, 2023 17:58
@erlend-aasland @AlexWaygood
Update Doc/library/sqlite3.rst
bedcfd3 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@erlend-aasland
... or iterator
ed08eab 
there's a slight difference
AlexWaygood
AlexWaygood reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

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

The set of "all Python iterables" is a superset of the set of "all Python sequences" and a superset of the set of "all Python sequences". All sequences are by definition also iterables; all iterators are by definition also iterables.

(sorry, reviewing on my phone — think I screwed up my last suggestion!)

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

Correct! I had to go re-read the glossary. Thanks for the heads-up.

@AlexWaygood
Copy link
Member

Correct! I had to go re-read the glossary. Thanks for the heads-up.

Pro tip: reading the source code for _collections_abc.py is often more informative than reading the glossary entry, I find!

AlexWaygood
AlexWaygood approved these changes Feb 10, 2023
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.

Looks great!

@erlend-aasland
Copy link
Contributor Author

Pro tip: reading the source code for _collections_abc.py is often more informative than reading the glossary entry, I find!

Ouch, we can't have it that way; in that case we need to improve the glossary. I haven't read a lot of the .py files in Lib; I'm more into the Modules dir :) But I'll take a look; thanks for the tip!

@erlend-aasland
Copy link
Contributor Author

Thanks for the helpful review; highly appreciated!

@erlend-aasland erlend-aasland merged commit 2037ebf into python:main Feb 10, 2023
@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 could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2037ebf81bd4bbe5421421b822bd57cfd665a1e9 3.10

@bedevere-bot
Copy link

GH-101803 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 Feb 10, 2023
@erlend-aasland erlend-aasland deleted the sqlite-docs/execute-params-best-practice branch February 10, 2023 17:54
@erlend-aasland
Copy link
Contributor Author

(I'll fix the 3.10 backport later today)

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 10, 2023
@erlend-aasland @AlexWaygood @miss-islington
Docs: use parameter list for sqlite3.Cursor.execute* (pythonGH-101782)
16e0b88 
(cherry picked from commit 2037ebf)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member

Ouch, we can't have it that way; in that case we need to improve the glossary.

I know what you mean, but I actually wonder if it's possible in this case to convey the same information as tersely and as precisely in the glossary. I find the inheritance structures in _collections_abc.py very descriptive and quite elegant. It's possible you could replicate that with some kind of diagram in the docs somewhere, but I'm not sure.

There's also the fact that it's nice to have all the collections-related concepts described together in a separate file. The glossary interspersed these terms with completely unrelated concepts (as a glossary should! This is its nature — but for understanding these concepts specifically, I find it unhelpful)

miss-islington added a commit that referenced this pull request Feb 10, 2023
@miss-islington @erlend-aasland @AlexWaygood
Docs: use parameter list for sqlite3.Cursor.execute* (GH-101782)
18313ec 
(cherry picked from commit 2037ebf)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
carljm added a commit to carljm/cpython that referenced this pull request Feb 10, 2023
@carljm
Merge branch 'main' into inlinecomp2
36b2917 
* main:
  Docs: Fix getstatus() -> getcode() typos (python#101296)
  Docs: use parameter list for sqlite3.Cursor.execute* (python#101782)
  pythongh-101763: Update bundled copy of libffi to 3.4.4 on Windows (pythonGH-101784)
  pythongh-101517: make bdb avoid looking up in linecache with lineno=None (python#101787)
  pythongh-101759: Update Windows installer to SQLite 3.40.1 (python#101762)
  pythongh-101277: Finalise isolating itertools (pythonGH-101305)
  pythongh-101759: Update macOS installer to SQLite 3.40.1 (python#101761)
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Feb 10, 2023
@erlend-aasland
[3.10] Docs: use parameter list for sqlite3.Cursor.execute* (pythonGH…
dca2822 
…-101782)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>.
(cherry picked from commit 2037ebf)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-bot
Copy link

GH-101808 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 Feb 10, 2023
erlend-aasland added a commit that referenced this pull request Feb 10, 2023
@erlend-aasland
[3.10] Docs: use parameter list for sqlite3.Cursor.execute* (GH-101782)…
a565cd5 
… (#101808)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

(cherry picked from commit 2037ebf)
@tom93 tom93 mentioned this pull request Nov 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

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

Assignees

@erlend-aasland erlend-aasland

Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erlend-aasland @AlexWaygood @miss-islington @bedevere-bot