FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence by erlend-aasland · Pull Request #101698 · python/cpython · GitHub

/ cpython Public

gh-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence - #101698

Merged
erlend-aasland merged 16 commits into
python:mainfrom
erlend-aasland:no-numeric-placeholder-for-you
Feb 15, 2023
ConversationCommits (16)ChecksFiles changed
Merged

gh-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence#101698
erlend-aasland merged 16 commits into
python:mainfrom
erlend-aasland:no-numeric-placeholder-for-you

Conversation

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

Copy link
Copy Markdown
Contributor

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 mentioned this pull request Feb 8, 2023
Closed

This comment was marked as outdated.

Sign in to view
erlend-aasland force-pushed the no-numeric-placeholder-for-you branch from 25365e9 to b693895 Compare February 8, 2023 17:49
erlend-aasland marked this pull request as ready for review February 8, 2023 18:08

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.

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 😶

erlend-aasland commented Feb 8, 2023
View reviewed changes
Comment thread Doc/library/sqlite3.rst Outdated
serhiy-storchaka reviewed Feb 9, 2023
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Modules/_sqlite/cursor.c Outdated
Comment thread Lib/test/test_sqlite3/test_dbapi.py
serhiy-storchaka approved these changes Feb 9, 2023
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Comment thread Doc/library/sqlite3.rst Outdated

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?

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

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A few more minor points

Comment thread Doc/whatsnew/3.12.rst Outdated
Comment thread Misc/NEWS.d/next/Library/2023-02-08-18-20-58.gh-issue-101693.4_LPXj.rst Outdated
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 Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
.. versionchanged:: 3.12
.. deprecated-removed:: 3.12 3.14

erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

AlexWaygood Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 :/

AlexWaygood Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

erlend-aasland Feb 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

AlexWaygood Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 🙂

erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Aight! I'll try to reword it.

erlend-aasland Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 🙂

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

AlexWaygood left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The docs LGTM!

erlend-aasland merged commit 8a2b7ee into python:main Feb 15, 2023
erlend-aasland deleted the no-numeric-placeholder-for-you branch February 15, 2023 05:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL