NotificationsYou must be signed in to change notification settings
Fork32.4k
Star67.9k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

erlend-aasland merged 8 commits intopython:mainfromerlend-aasland:sqlite/deprecate-optional-parameters

Aug 15, 2023

Merged

gh-93057: Deprecate positional use of optional sqlite3.connect() params#107948

erlend-aasland merged 8 commits intopython:mainfromerlend-aasland:sqlite/deprecate-optional-parameters

Aug 15, 2023

Conversation

Copy link

Contributor

erlend-aasland commentedAug 14, 2023•
edited by github-actionsbot
Loading

Issue:Make optional parameters of sqlite3.connect() keyword-only #93057

📚 Documentation preview 📚:https://cpython-previews--107948.org.readthedocs.build/

pythongh-93057: Deprecate positional use of optional sqlite3.connect(…

40996ef

…) params

erlend-aasland requested a review fromberkerpeksag as acode owner

August 14, 2023 17:50

erlend-aasland requested a review fromserhiy-storchaka

August 14, 2023 17:50

bedevere-bot mentioned this pull request

Aug 11, 2023

Make optional parameters of sqlite3.connect() keyword-only#93057

Closed

bedevere-bot added the awaiting core review label

Aug 14, 2023

This comment was marked as outdated.

serhiy-storchaka reviewed

Aug 14, 2023

View reviewed changes

Doc/whatsnew/3.13.rst OutdatedShow resolvedHide resolved

erlend-aasland added3 commits

August 14, 2023 21:45

Fix test and news entries

b84ee0e

regen clinic

6ab04fd

Address review: don't repeat the what's new entry

01c4067

erlend-aasland commented

Aug 14, 2023

View reviewed changes

Lib/test/test_sqlite3/test_factory.pyShow resolvedHide resolved

serhiy-storchaka reviewed

Aug 15, 2023

View reviewed changes

Copy link

Member

serhiy-storchaka left a 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 theConnection constructor. But we want also to deprecate them inconnect(). Currently you can bypass the deprecations if use theConnection 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 inconnect(), we will get two deprecation warning.

Copy link

ContributorAuthor

erlend-aasland commentedAug 15, 2023

There is an issue here. Positional parameters are only deprecated in theConnection constructor. But we want also to deprecate them inconnect(). Currently you can bypass the deprecations if use theConnection subclass:

Ah, of course. That's tricky.

erlend-aasland added2 commits

August 15, 2023 08:57

Manually insert deprecation warning for sqlite3.connect

de7e87c

Pull in main

6f3e5c6

serhiy-storchaka reviewed

Aug 15, 2023

View reviewed changes

Copy link

Member

serhiy-storchaka left a 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 forconnect() 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 inconnect(), then (since 3.15) inConnection. Not sure if it it is worth.

We can also convert all positional arguments inconnect() into keyword arguments forfactory. It is a lot of complex code.

Lib/test/test_sqlite3/test_factory.py OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

erlend-aasland commentedAug 15, 2023

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.

Copy link

ContributorAuthor

erlend-aasland commentedAug 15, 2023

mention this change in What's New and NEWS entries.

Is the existing NEWS and What's New entries insufficient? In what way?

Copy link

Member

serhiy-storchaka commentedAug 15, 2023

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".

Also test module level connect()

4018bd9

Copy link

ContributorAuthor

erlend-aasland commentedAug 15, 2023

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.