GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43094: Update sqlite3 docs to match implementation by erlend-aasland · Pull Request #24489 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Feb 9, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue43094

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Erlend E. Aasland
bpo-43094: Update docs to match implementation
be12197
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner February 9, 2021 11:03
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 9, 2021
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Feb 9, 2021
@erlend-aasland

erlend-aasland commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

@berkerpeksag Would you mind reviewing this?

skip news, I presume.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 added the skip news label Feb 9, 2021
Fidget-Spinner
Fidget-Spinner approved these changes Feb 9, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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:

cpython/Modules/_sqlite/clinic/connection.c.h

Line 97 in 3ccef1c

"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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 9, 2021
@terryjreedy

terryjreedy commented Feb 9, 2021

Copy link
Copy Markdown
Member

This is a substantive change, not a typo or grammar fix, so I would not merge without a blurb.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Feb 9, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Sorry if this isn't related, but I just noticed the docstrings on some other functions don't match either:
Eg create_collation docstring:

cpython/Modules/_sqlite/clinic/connection.c.h

Line 623 in 3ccef1c

"create_collation($self, name, callback, /)\n"

Ref:

cpython/Modules/_sqlite/clinic/connection.c.h

Line 633 in 3ccef1c

PyObject *name, PyObject *callable);

Meanwhile the one on python docs is correct
https://docs.python.org/3/library/sqlite3.html#sqlite3.Connection.create_collation

I wonder if this is intentional?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

Sorry if this isn't related, but I just noticed the docstrings on some other functions don't match either:
Eg create_collation docstring:
[...]

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 sqlite3 and add any discrepancies to the PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

This is a substantive change, not a typo or grammar fix, so I would not merge without a blurb.

Noted. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland added 9 commits February 9, 2021 20:03
Erlend E. Aasland
Add NEWS
a046469
Erlend E. Aasland
Address review: Fix create_collation docstring
30c753f
Erlend E. Aasland
Fix sqlite3.register_converter() docstring
4dd7ec4
Erlend E. Aasland
Fix sqlite3.register_adapter() docstring
6d81622
Erlend E. Aasland
Fix sqlite3.complete_statement() docstring
fe3fda6
Erlend E. Aasland
Fix sqlite3.enable_callback_tracebacks() docstring
2309e61
Erlend E. Aasland
Fix sqlite3.adapt() docstring
85fa271
Erlend E. Aasland
Revert sqlite3.complete_statement() docstring
22a94be
Erlend E. Aasland
Fix sqlite3.complete_statement() docs
92a6efa
@erlend-aasland

erlend-aasland commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Feb 9, 2021

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Feb 10, 2021

Copy link
Copy Markdown
Contributor Author

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.

Makes sense!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@terryjreedy terryjreedy added the DO-NOT-MERGE label Feb 10, 2021
@terryjreedy

terryjreedy commented Feb 10, 2021

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Feb 10, 2021

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland closed this Feb 10, 2021
@erlend-aasland
erlend-aasland deleted the bpo-43094 branch February 10, 2021 08:40
@nchammas nchammas mannequin mentioned this pull request May 16, 2022
sqlite3 signature discrepancies between documentation and implementation #87260
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review DO-NOT-MERGE docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @terryjreedy @Fidget-Spinner @corona10 @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.