-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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-93057: Deprecate positional use of optional sqlite3.connect() params #107948
gh-93057: Deprecate positional use of optional sqlite3.connect() params #107948
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
There is an issue here. Positional parameters are only deprecated in the Connection
constructor. But we want also to deprecate them in connect()
. Currently you can bypass the deprecations if use the Connection
subclass:
class MyConnection(Connection):
def __init__(self, database, timeout=5.0, ...):
super().__init__(database, timeout=timeout, ...)
connect(":memory:", 10.0, ..., factory=MyConnection)
But if we add the second deprecation warning in connect()
, we will get two deprecation warning.
Ah, of course. That's tricky. |
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.
Please add a test for connect()
and mention this change in What's New and NEWS entries.
It is a wart that we will get two warnings, but there is no simple way to avoid it, except applying deprecations one at a time -- first in connect()
, then (since 3.15) in Connection
. Not sure if it it is worth.
We can also convert all positional arguments in connect()
into keyword arguments for factory
. It is a lot of complex code.
No, that is a backwards incompatible change. |
Is the existing NEWS and What's New entries insufficient? In what way? |
Ah, they already mention connect()! Then simply add "and the Connection constructor". |
Yeah, that's useful information. I'll add that; thanks. |
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.
Thanks for the review, Serhiy. Much appreciated. |
@felixxm, sorry, I forgot to ping you; I hope not these deprecations will create havoc for Django. |
All good, Django doesn't pass these positional params, only a database name. Thanks 👍 |
📚 Documentation preview 📚: https://cpython-previews--107948.org.readthedocs.build/