Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-93057: Deprecate positional use of optional sqlite3.connect() params#107948
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
erlend-aasland commented Aug 14, 2023 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka left a comment
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.
erlend-aasland commented Aug 15, 2023
Ah, of course. That's tricky. |
serhiy-storchaka left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland commented Aug 15, 2023
No, that is a backwards incompatible change. |
erlend-aasland commented Aug 15, 2023
Is the existing NEWS and What's New entries insufficient? In what way? |
serhiy-storchaka commented Aug 15, 2023
Ah, they already mention connect()! Then simply add "and the Connection constructor". |
erlend-aasland commented Aug 15, 2023
Yeah, that's useful information. I'll add that; thanks. |
serhiy-storchaka left a comment
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.
erlend-aasland commented Aug 15, 2023
Thanks for the review, Serhiy. Much appreciated. |
erlend-aasland commented Aug 15, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@felixxm, sorry, I forgot to ping you; I hope not these deprecations will create havoc for Django. |
felixxm commented Aug 15, 2023
All good, Django doesn't pass these positional params, only a database name. Thanks 👍 |
📚 Documentation preview 📚: https://cpython-previews--107948.org.readthedocs.build/