-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
bpo-43094: Update sqlite3 docs to match implementation #24489
Conversation
@berkerpeksag Would you mind reviewing this? skip news, I presume. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not Berker, but this LGTM :).
For convenience:
create_function:
"create_function($self, /, name, narg, func, *, deterministic=False)\n" |
create_aggregate:
cpython/Modules/_sqlite/clinic/connection.c.h
Line 160 in 3ccef1c
"create_aggregate($self, /, name, n_arg, aggregate_class)\n" |
Which seems to match up with the changes in the docs.
This is a substantive change, not a typo or grammar fix, so I would not merge without a blurb. |
Sorry if this isn't related, but I just noticed the docstrings on some other functions don't match either: cpython/Modules/_sqlite/clinic/connection.c.h Line 623 in 3ccef1c
Ref: cpython/Modules/_sqlite/clinic/connection.c.h Line 633 in 3ccef1c
Meanwhile the one on python docs is correct I wonder if this is intentional? |
That seems to be a regression from #23341. Good catch! I guess it's fine to include that one in this PR. I'll do an audit of all docstrings/docs/specs in |
Noted. Thanks! |
@Fidget-Spinner: I've done an audit of the module level functions. Should this be backported to 3.9 and 3.8? If so, I will have to split this PR in two: one that contains docstring fixes (regressions from my recent AC change), and one that contains documentation fixes that should be backported. |
Thanks for fixing them! My hunch is the regression fixes should go into another PR with the bpo number same as the PR which caused the regression, while the backportable stuff remains in this PR. However I am not an sqllite3 expert by any means, so I wouldn't be surprised if this hunch is wrong. |
Makes sense! |
Do not merge yet because there is still discussion on pydev, and some disagreement, as to precisely the fix should be. In particular, can we make the proper names keyword only? Please make sure you read the thread and wait for consensus. Regression fixes do not have to be attached to the regressing issue. It has been done both ways, with a note on the original issue if done on a separate issue. |
Yes, I'm following the thread. I think I'll close this PR bco. noise, and open a new one when there's concensus. |
https://bugs.python.org/issue43094