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-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence by erlend-aasland · Pull Request #101698 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Feb 8, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: sqlite3: issue a warning if a sequence of params are used with named placeholders in queries #101693

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added the awaiting core review label Feb 8, 2023
@erlend-aasland erlend-aasland changed the title Add NEWS gh-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence Feb 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 8, 2023
sqlite3: issue a warning if a sequence of params are used with named placeholders in queries #101693
Closed
@erlend-aasland

This comment was marked as outdated.

Sign in to view
@erlend-aasland
pythongh-101698: Deprecate using named placeholders with sequences
b693895
@erlend-aasland
erlend-aasland force-pushed the no-numeric-placeholder-for-you branch from 25365e9 to b693895 Compare February 8, 2023 17:49
@erlend-aasland
Update docs
483e4d6
@erlend-aasland
erlend-aasland marked this pull request as ready for review February 8, 2023 18:08
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner February 8, 2023 18:08
@erlend-aasland
erlend-aasland requested a review from AlexWaygood February 8, 2023 18:08
@erlend-aasland

erlend-aasland commented Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

Alex, would you mind taking a look at the docs/NEWS/What's New changes? I feel the sentences grew to be a little bit too complex.

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

erlend-aasland commented Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

Also, the text in the deprecation warning is perhaps a little bit too long 😶

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland commented Feb 8, 2023
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

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

erlend-aasland added 3 commits February 8, 2023 19:25
@erlend-aasland
Document deprecation in reference
e826428
@erlend-aasland
Also test executemany
132a6ee
@erlend-aasland
Revert "Also test executemany"
31cc188
executemany only works with data-modifying queries
serhiy-storchaka
serhiy-storchaka reviewed Feb 9, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

I can't think of a case where a warning would be better than an exception, but so be it.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/_sqlite/cursor.c Resolved

Uh oh!

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

Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

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

erlend-aasland added 3 commits February 9, 2023 12:13
@erlend-aasland
Sync with main
660a742
@erlend-aasland
Address review: add test for query with more than one named param
a18d0bb
@erlend-aasland
Address review: add PyErr_WarnFormat error handling
3f94201
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka February 9, 2023 11:22
serhiy-storchaka
serhiy-storchaka approved these changes Feb 9, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Sorry, something went wrong.

Uh oh!

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

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

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Feb 9, 2023
@erlend-aasland
Sync with main
997ccc6
@erlend-aasland erlend-aasland added the DO-NOT-MERGE label Feb 10, 2023
erlend-aasland added 3 commits February 14, 2023 13:01
@erlend-aasland
Pull in main
cb091af
@erlend-aasland
Fixup
450cdcc
@erlend-aasland
Apply Serhiy's suggestion
841e8a4
serhiy-storchaka
serhiy-storchaka approved these changes Feb 14, 2023
View reviewed changes
@erlend-aasland

erlend-aasland commented Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for your review, Serhiy; highly appreciated.

@AlexWaygood, do you still want to take a look?

All reactions

Sorry, something went wrong.

Uh oh!

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

AlexWaygood
AlexWaygood reviewed Feb 14, 2023
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

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

erlend-aasland added 3 commits February 14, 2023 21:24
@erlend-aasland
Pull in main
3e7b680
@erlend-aasland
Address Alex's review
38d1110
@erlend-aasland
Also amend NEWS and What's New
cf3cfc8
AlexWaygood
AlexWaygood reviewed Feb 14, 2023
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.

A few more minor points

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Library/2023-02-08-18-20-58.gh-issue-101693.4_LPXj.rst Resolved

Uh oh!

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

Comment thread Doc/library/sqlite3.rst Outdated
and there is no open transaction,
a transaction is implicitly opened before executing *sql*.

.. versionchanged:: 3.12

@AlexWaygood AlexWaygood Feb 14, 2023

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.

Suggested change
.. versionchanged:: 3.12
.. deprecated-removed:: 3.12 3.14

Sorry, something went wrong.

Uh oh!

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

All reactions

@erlend-aasland erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

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.

Are you sure about that? The API itself is not deprecated, we're just changing it. I'm not sure what's the recommended practice here.

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Feb 14, 2023

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.

Yeah, I know what you mean. I feel like ideally this notice would be phrased along the lines of "X behaviour/practice is now deprecated" rather than "a DeprecationWarning is now emitted". That would be more to-the-point, and it would also work more naturally with this directive (X behaviour/practice is deprecated in 3.12, and will be removed entirely in 3.14).

But I was struggling to come to with a concrete suggestion for how to reword these notices :/

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Feb 14, 2023

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.

(The whatsnew and NEWS entries look great btw, it's just the notices in the API docs that feel slightly clunky to me)

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

@erlend-aasland erlend-aasland Feb 14, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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.

I feel like ideally this notice would be phrased along the lines of "X behaviour/practice is now deprecated" rather than "a DeprecationWarning is now emitted". That would be more to-the-point, and it would also work more naturally with this directive (X behaviour/practice is deprecated in 3.12, and will be removed entirely in 3.14).

Yes, but documented as deprecated and emitting a DeprecationWarning are similar, but not equal, things 🙂 With the former, we don't need to emit a warning in the code.

Sorry, something went wrong.

Uh oh!

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

All reactions

@AlexWaygood AlexWaygood Feb 14, 2023

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.

To your original question, btw: I think it's pretty standard to use .. deprecated or .. deprecated-removed, even if it's just a particular usage of an API, rather than the API itself. See e.g. https://docs.python.org/3.12/library/asyncio-policy.html#asyncio.DefaultEventLoopPolicy, where the directive is used even though the class itself hasn't been deprecated at all; or #19867, which deprecated just a specific parameter; or lots of other examples in our docs 🙂

Sorry, something went wrong.

Uh oh!

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

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

@erlend-aasland erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

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.

Aight! I'll try to reword it.

Sorry, something went wrong.

Uh oh!

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

All reactions

@erlend-aasland erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

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.

I've updated it to use deprecated-removed, and I put in an extra line regarding what happens in 3.14. I'm too tired to reword the text 🙂

Sorry, something went wrong.

Uh oh!

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

AlexWaygood 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
Use deprecated-removed
1687b65
AlexWaygood
AlexWaygood approved these changes Feb 14, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

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

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.

The docs LGTM!

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
erlend-aasland merged commit 8a2b7ee into python:main Feb 15, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 15, 2023
@erlend-aasland
erlend-aasland deleted the no-numeric-placeholder-for-you branch February 15, 2023 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

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

Assignees

No one assigned

Labels

DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@erlend-aasland @serhiy-storchaka @AlexWaygood @bedevere-bot
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.