Repository navigation
Clean up sqlite3.Connection APIs #108278
Description
Feature or enhancement
Has this already been discussed elsewhere?
https://discuss.python.org/t/clean-up-some-sqlite3-apis/32093
Links to previous discussion of this feature:
Proposal:
The sqlite3.Connection class has groups of similar APIs with inconsistent parameter specs.
Create user-defined function APIs:
create_function(name, narg, func, *, deterministic=False)create_aggregate(name, /, n_arg, aggregate_class)create_window_function(name, num_params, aggregate_class, /)create_collation(name, callable, /)
Set callback APIs:
set_authorizer(authorizer_callback)set_progress_handler(progress_handler, n)set_trace_callback(trace_callback)
For all APIs but create_function, I suggest to make all parameters positional-only; for create_function, I suggest to make the three first parameters positional-only:
Create user-defined function APIs:
create_function(name, nargs, callable, /, *, deterministic=False)create_aggregate(name, nargs, aggregate_class, /)create_window_function(name, nargs, aggregate_class, /)create_collation(name, callable, /)
Set callback APIs:
set_authorizer(authorizer_callback, /)set_progress_handler(progress_handler, /, n)set_trace_callback(trace_callback, /)
Obviously, create_window_function stays as it is.
UPDATE: I noticed the docs are wrong about create_collation; it's signature is actually create_collation(name, callable, /).
Activity
erlend-aasland commentedon Aug 22, 2023
cc. @serhiy-storchaka
pythongh-108278: Clean up some sqlite3 connection APIs
[-]Clean up sqlite3 APIs[/-][+]Clean up sqlite3.Connection APIs[/+]serhiy-storchaka commentedon Aug 22, 2023
Why keep
ninset_progress_handler()as keyword-or-positional parameter? It is not very expressive name.erlend-aasland commentedon Aug 22, 2023
I did consider it, and I'm still considering it :) I'm not sure what makes for the better API.
con.set_progress_handler(func, 10)con.set_progress_handler(func, n=10)serhiy-storchaka commentedon Aug 23, 2023
Or maybe
con.set_progress_handler(func, num_ops=10)?nmay not be the best name.On other hand,
set_progress_handler()has the order of parameters opposite tosqlite3_progress_handler(), it can cause confusion, so someone can call it ascon.set_progress_handler(n=10, progress_handler=func).So I am not sure about these changes. On other hand, having different names
narg,n_argandnum_paramsfor virtually the same parameter is ridiculous. Andnamewhich can or cannot be passed by keyword. Adfuncandcallablewhich has virtually the same meaning in Python. So I agree with the first part of changes.Alternatively we can rename keyword arguments with keeping deprecated alias (#108271), but first we need to agree on names. It is easier to simply deprecate passing by keyword.
erlend-aasland commentedon Aug 23, 2023
Yes, it is easier to deprecate passing by keyword, and I also think it makes for a cleaner API.
Regarding similarity to SQLite C API; I'm not sure it makes sense to align our APIs with those of SQLite. The sqlite3 extension module is a DB API implementation; it does not aim to be a Python shim over the SQLite C API (like apsw).
I'll split my PR in two, so there is one PR for each API group; it will be easier to reason about.
erlend-aasland commentedon Aug 23, 2023
I agree; generally, I'm not a big fan of 1-letter variable/parameter names.
num_opsornopswould be better. As you say, we'd need to resolve #108271 if we want to be able to rename a keyword parameter.gh-108278: Deprecate passing the three first params as keyword args f…
pythongh-108278: Deprecate passing the three first params as keyword …
pythongh-108278: Deprecate passing the first param of sqlite3.Connect…
erlend-aasland commentedon Aug 29, 2023
@serhiy-storchaka, I created a PR for the callback handlers. You raised a question regarding the
nparameter ofset_progress_handler, and also regarding the similarity to the SQLite C API.Regarding the latter, I already replied that I don't think the order of parameters in the SQLite C API should be directing the order of parameters in the sqlite3 API; the sqlite3 module is not apsw.
Regarding my suggested omission of the
nparameters inset_progress_handler, I am not sure what makes for the best API:set_progress_handler(func, n=100)set_progress_handler(func, 100)nis a single letter; one can argue that it does not convey a lot of meaning;nwill probably be interpreted as meaning "count", but it is still unknown what is counted :) Obviouslyninstrornum_instructionswould have been a better choice. The easiest solution is to make it a positional parameter instead.Also, the docstring of
set_progress_handlercan be greatly improved, so perhapsnis fine, as long as the docstring explains it.gh-108278: Deprecate passing the first param of sqlite3.Connection ca…
erlend-aasland commentedon Aug 29, 2023
Thanks for your input and reviews, Serhiy.
Merge branch 'main' into icompexc