Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-43094: Update sqlite3 docs to match implementation#24489
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
erlend-aasland commented Feb 9, 2021 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland commented Feb 9, 2021
@berkerpeksag Would you mind reviewing this? skip news, I presume. |
Fidget-Spinner left a comment
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.
terryjreedy commented Feb 9, 2021
This is a substantive change, not a typo or grammar fix, so I would not merge without a blurb. |
Fidget-Spinner commented Feb 9, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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? |
erlend-aasland commented Feb 9, 2021
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 |
erlend-aasland commented Feb 9, 2021
Noted. Thanks! |
erlend-aasland commented Feb 9, 2021
@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. |
Fidget-Spinner commented Feb 9, 2021
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. |
erlend-aasland commented Feb 10, 2021
Makes sense! |
terryjreedy commented Feb 10, 2021
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. |
erlend-aasland commented Feb 10, 2021
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