Skip to content

gh-107938: Synchonise the signature of of sqlite3.connect and sqlite3.Connection.__init__ #107939

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

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 14, 2023

@erlend-aasland
Copy link
Contributor Author

Suggesting to backport this, in order to easier backport docstring updates.

@erlend-aasland
Copy link
Contributor Author

cc. @serhiy-storchaka, if you want to look this over; it is a trivial change

Copy link
Member

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

This is an interesting trick, but in future they can divergent.

Connection.__init__ and __init__ in its subclasses currently require the factory parameter to be supported, even if it is ignored. This is both a wart and an artifact of implementation. It may be removed in the future, and from then on the connect and Connection.__init__ signatures will be different.

It is a long way, and I think that it can only happen in distant future, but you should be aware of consequences.

@erlend-aasland
Copy link
Contributor Author

Yes, that is true, but as you say, it is a long way into the future. For now, it makes it simpler for us to do stuff like deprecating positional use of parameters.

@erlend-aasland erlend-aasland merged commit 6fbaba5 into python:main Aug 14, 2023
@miss-islington

This comment was marked as outdated.

@miss-islington

This comment was marked as outdated.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 14, 2023
…qlite3.Connection.__init__ (pythonGH-107939)

(cherry picked from commit 6fbaba5)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@bedevere-bot

This comment was marked as outdated.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 14, 2023
@serhiy-storchaka
Copy link
Member

BTW, after making all optional arguments keyword-only, why not implement connect() in Python?

def connect(database, *, factory=Connection, **kwargs):
    return factory(database, **kwargs)

@erlend-aasland erlend-aasland deleted the sqlite/clone-init branch August 14, 2023 15:24
@erlend-aasland
Copy link
Contributor Author

BTW, after making all optional arguments keyword-only, why not implement connect() in Python?

def connect(database, *, factory=Connection, **kwargs):
    return factory(database, **kwargs)

That is a very good idea.

BTW, I've toyed the idea of with turning _sqlite3 into a small extension module that simply exposes the relevant SQLite APIs, and then implement Lib/sqlite3.py using _sqlite3. It would probably have a performance impact, but it would make maintenance easier.

@bedevere-bot

This comment was marked as outdated.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 14, 2023
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Aug 14, 2023
…t and sqlite3.Connection.__init__ (python#107939)

(cherry picked from commit 6fbaba5)
@erlend-aasland
Copy link
Contributor Author

Ah, of course, we cannot backport this because the clone __init__ feature in Argument Clinic is only present in main. Well, backporting is not very important; the spec of sqlite3.connect and sqlite3.Connection.__init__ should not change in 3.12 and 3.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants