GeezerVison is showing github.com — Clean up sqlite3.Connection APIs · Issue #108278 · python/cpython
Clean up sqlite3.Connection APIs #108278DescriptionFeature 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:
#87260 (comment)
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, /).
Linked PRs
Activityerlend-aasland commented on Aug 22, 2023 ContributorAuthor More actions changed the title [-]Clean up sqlite3 APIs[/-] [+]Clean up sqlite3.Connection APIs[/+] on Aug 22, 2023serhiy-storchaka commented on Aug 22, 2023 Why keep n in set_progress_handler() as keyword-or-positional parameter? It is not very expressive name. erlend-aasland commented on Aug 22, 2023 ContributorAuthor More actions
Why keep n in set_progress_handler() as keyword-or-positional parameter? It is not very expressive name.
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 commented on Aug 23, 2023 Or maybe con.set_progress_handler(func, num_ops=10)? n may not be the best name.
On other hand, set_progress_handler() has the order of parameters opposite to sqlite3_progress_handler(), it can cause confusion, so someone can call it as con.set_progress_handler(n=10, progress_handler=func).
So I am not sure about these changes. On other hand, having different names narg, n_arg and num_params for virtually the same parameter is ridiculous. And name which can or cannot be passed by keyword. Ad func and callable which 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 commented on Aug 23, 2023 ContributorAuthor More actions 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 commented on Aug 23, 2023 ContributorAuthor More actions
n may not be the best name.
I agree; generally, I'm not a big fan of 1-letter variable/parameter names. num_ops or nops would be better. As you say, we'd need to resolve #108271 if we want to be able to rename a keyword parameter. added a commit that references this issue on Aug 28, 2023gh-108278: Deprecate passing the three first params as keyword args f… erlend-aasland commented on Aug 29, 2023 ContributorAuthor More actions @serhiy-storchaka, I created a PR for the callback handlers. You raised a question regarding the n parameter of set_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 n parameters in set_progress_handler, I am not sure what makes for the best API:
set_progress_handler(func, n=100)set_progress_handler(func, 100)
n is a single letter; one can argue that it does not convey a lot of meaning; n will probably be interpreted as meaning "count", but it is still unknown what is counted :) Obviously ninstr or num_instructions would have been a better choice. The easiest solution is to make it a positional parameter instead.
Also, the docstring of set_progress_handler can be greatly improved, so perhaps n is fine, as long as the docstring explains it. added a commit that references this issue on Aug 29, 2023gh-108278: Deprecate passing the first param of sqlite3.Connection ca… erlend-aasland commented on Aug 29, 2023 ContributorAuthor More actions Thanks for your input and reviews, Serhiy.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|