Repository navigation
Make optional parameters of sqlite3.connect() keyword-only #93057
Description
Activity
The problem is that sqlite3.connect() was converted to Argument Clinic, and it was much easier to add deprecation warning in the old code.
Can we enhance AC with such functionality first?
Reacted by Jelle Zijlstra and ShantanuCan we enhance AC with such functionality first?
It would be not easy. 😫
It would be not easy. 😫
That I assumed 🙂 Do you have a coarse outline in mind of how such a change could be implemented?
Can we enhance AC with such functionality first?
It would be not easy. 😫
Continuing the AC digression :)
I made a proof-of-concept of such AC functionality here: erlend-aasland#37
It is a very crude implementation; there's a lot of corner cases missing checks. See details for more info. Feel free to play around with it, and/or improve it (or even finish it). We make an issue to follow up this now.Details
I introduced a new special symbol,
x, that means "all optional parameters from now on are deprecated as positional parameters". We can use any other symbol or string. For exampled/(deprecated positional),-/,deprecated/. I liked the brevity ofx, and how it visually kinda resembles*.Example AC code:
/*[clinic input] _sqlite3.connect as pysqlite_connect database: object timeout: double = 5.0 detect_types: int = 0 isolation_level: object = NULL check_same_thread: bool(accept={int}) = True factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType x cached_statements: int = 128 uri: bool = False Opens a connection to the SQLite database file database. You can use ":memory:" to open a database connection to a database that resides in RAM instead of on disk. [clinic start generated code]*/
It is recommended to make optional rarely used arguments keyword-only. I do not think it will break much code, but we need a deprecation period for this.
I suggest we make all but the database argument keyword-only. If my AC hack is a possible way forward1, we can do this in 3.12.
Also, the database argument should be positional-only.
Footnotes
-
It is a hack; I didn't think through all possible scenarios, or even if it is the best path forward. ↩
-
Argument Clinic now has support for deprecating positional use of positional-or-keyword parameters. However, there are still some prerequisites that must be in place before we can continue with this issue:
- Currently, the clinic code in Modules/_sqlite uses
module _sqlite3. This means that deprecation warnings will mention_sqlite3.connect(), notsqlite3.connect(). This can be confusing for users. I suggest we change the clinic code to usemodule sqlite3instead. This change should not result in any behaviour change. - Next, we have some technical dept when it comes to
sqlite3.connectandsqlite3.Connection.__init__. Currently, the latter uses clinic, and the former does not use clinic. However, it issqlite3.connectthat needs the docstring, notsqlite3.Connection.__init__. We should make it so that the docstring fromsqlite3.Connection.__init__is used forsqlite3.connectand output to a separate file, for inclusion by Module/_sqlite/module.c. - In order to achieve 2. properly, we must make it possible for clinic.py to clone
sqlite3.Connection.__init__tosqlite3.connect.
This is roughly what the relevant parts of Modules/_sqlite/connection.c will look like:
/*[clinic input] sqlite3.Connection.__init__ as pysqlite_connection_init database: object * [from 3.15] timeout: double = 5.0 detect_types: int = 0 isolation_level: IsolationLevel = "" check_same_thread: bool = True factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType cached_statements as cache_size: int = 128 uri: bool = False * autocommit: Autocommit(c_default='LEGACY_TRANSACTION_CONTROL') = sqlite3.LEGACY_TRANSACTION_CONTROL [clinic start generated code]*/ static int pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database, double timeout, int detect_types, const char *isolation_level, int check_same_thread, PyObject *factory, int cache_size, int uri, enum autocommit_mode autocommit) /*[clinic end generated code: output=cba057313ea7712f input=a0949fb85339104d]*/ { // __init__ function is here; fast forward ... } /*[clinic input] # Save the clinic output config. output push # Create a new destination 'connect' for the docstring and methoddef only. destination connect new file '{dirname}/clinic/_sqlite3.connect.c.h' output everything suppress output docstring_definition connect output methoddef_define connect # Now, define the connect function. sqlite3.connect as pysqlite_connect = sqlite3.Connection.__init__ [clinic start generated code]*/ /*[clinic end generated code: output=da39a3ee5e6b4b0d input=7913cd0b3bfc1b4a]*/ /*[clinic input] # Restore the clinic output config. output pop [clinic start generated code]*/
Then, in Modules/_sqlite/module.c:
#include "clinic/_sqlite3.connect.c.h" static PyObject * pysqlite_connect(PyObject *module, PyObject *const *args, Py_ssize_t nargsf, PyObject *kwnames) { // ... } // ... and then: static PyMethodDef module_methods[] = { PYSQLITE_ADAPT_METHODDEF PYSQLITE_COMPLETE_STATEMENT_METHODDEF PYSQLITE_CONNECT_METHODDEF // <= this now comes from clinic/_sqlite3.connect.c.h PYSQLITE_ENABLE_CALLBACK_TRACE_METHODDEF PYSQLITE_REGISTER_ADAPTER_METHODDEF PYSQLITE_REGISTER_CONVERTER_METHODDEF {NULL, NULL} };
- Currently, the clinic code in Modules/_sqlite uses
- added 3 commits that reference this issue
on Aug 11, 2023 - moved this from Backwards compatibility issues to Done in sqlite3 issues
on Aug 15, 2023 Thanks for suggesting this feature and helping land it (and all its implications in Argument Clinic), @serhiy-storchaka. Very helpful, and a good learning experience.
For the record, it was the Argument Clinic implications of this feature that spawned the desire to add typing to clinic.py (#104050), which in turn got both @AlexWaygood and me highly involved with Argument Clinic. We've now ended up in a position where more core devs have better knowledge of the inner workings of clinic.py; this is a good thing. Thanks, again!
Reacted by Alex WaygoodAlso, the database argument should be positional-only.
If this is desired, we should create a new issue for it. cc. @serhiy-storchaka
sqlite3.connect()has a large number of parameters: 1 required and 7 optional. It is much more convenient to pass optional arguments by keyword, because you can pass only values which differs from default. And if you pass positional arguments, it is easy to make an error and pass value as wrong argument.It is recommended to make optional rarely used arguments keyword-only. I do not think it will break much code, but we need a deprecation period for this.
The problem is that
sqlite3.connect()was converted to Argument Clinic, and it was much easier to add deprecation warning in the old code.Linked PRs