GeezerVison

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

GeezerVison is showing github.com — gh-95235: Add explicit parameter list to some sqlite3 methods by erlend-aasland · Pull Request #95240 · python/cpython

Conversation

erlend-aasland commented Jul 25, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Issue: Document all parameters of all sqlite3 functions/methods #95235

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-95235: Add explicit parameter list to some sqlite3 methods

1d030fa

erlend-aasland requested a review from berkerpeksag as a code owner

July 25, 2022 11:43

erlend-aasland requested a review from CAM-Gerlach

July 25, 2022 11:43

bedevere-bot added docs

Documentation in the Doc dir

skip news awaiting core review labels

Jul 25, 2022

erlend-aasland commented

Jul 25, 2022

View reviewed changes

Comment thread

Doc/library/sqlite3.rst Resolved

Uh oh!

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

erlend-aasland commented

Jul 25, 2022

View reviewed changes

Comment thread

Doc/library/sqlite3.rst Resolved

Uh oh!

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

erlend-aasland added 2 commits

July 25, 2022 13:45


          Fix n_arg param of create_aggregate

f20107f


          Fix num_params of create_window_func

3a9b6f2

erlend-aasland added needs backport to 3.10 needs backport to 3.11 only security fixes labels

Jul 25, 2022

CAM-Gerlach reviewed

Jul 25, 2022

View reviewed changes

CAM-Gerlach 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.

Great stuff, thanks.

Do you think its worth adding a mention to the relevant parameters that are keyword or (especially, since their name is given) positional-only?

Also, given they are positional only, would it make sense to rename the narg/n_arg/num_params parameter, which is otherwise identical, to be consistant across the three functions? There's no obvious reason why it has a different name in each despite having a seemingly identical purpose and behavior.

Sorry, something went wrong.

Uh oh!

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

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

erlend-aasland commented Jul 25, 2022

Copy link

Copy Markdown

Contributor Author

Do you think its worth adding a mention to the relevant parameters that are keyword or (especially, since their name is given) positional-only?

I'm not sure it's needed. The signature is just above the parameter list, so IMO there is no need. I have no strong opinion about it though; I could easily be convinced otherwise.

Also, given they are positional only, would it make sense to rename the narg/n_arg/num_params parameter, which is otherwise identical, to be consistant across the three functions?

But it's not: create_aggregate has only one position-only parameter; n_arg can be specified as a keyword. I do dislike the current situation though. Personally I'd go with simply n or nargs.

CAM-Gerlach reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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


          Apply suggestions from code review

c49508c

Co-authored-by: CAM Gerlach <[email protected]>

erlend-aasland commented Jul 25, 2022

Copy link

Copy Markdown

Contributor Author

Suggestions applied; a great improvement. Thanks again.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland and others added 4 commits

July 25, 2022 21:01


          Whitespace lint and fixup connect()

0b2bc53


          Fixup rtype of blobopen

5e6f614


          Update Doc/library/sqlite3.rst

Co-authored-by: CAM Gerlach <[email protected]>


          Add :raises: for create_function

6d1aeab

CAM-Gerlach approved these changes

Jul 26, 2022

View reviewed changes

CAM-Gerlach 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.

LGTM now, thanks @erlend-aasland

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

erlend-aasland merged commit 5012bed into python:main

Jul 26, 2022

miss-islington commented Jul 26, 2022

Copy link

Copy Markdown

Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the awaiting core review label

Jul 26, 2022

erlend-aasland deleted the sqlite-doc-parameter-lists branch

July 26, 2022 06:29

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 26, 2022


          pythongh-95235: Add explicit parameter list to some sqlite3 methods (p…

517090f

…ythonGH-95240)

Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 5012bed)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

bedevere-bot removed the needs backport to 3.11 only security fixes label

Jul 26, 2022

miss-islington commented Jul 26, 2022

Copy link

Copy Markdown

Contributor

Sorry, @erlend-aasland, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5012bedc7ca7d78b5a42bf60eef55bac263f4060 3.10

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot commented Jul 26, 2022

Copy link

Copy Markdown

GH-95263 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned erlend-aasland

Jul 26, 2022

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request

Jul 26, 2022


          [3.10] pythongh-95235: Add explicit parameter list to some sqlite3 me…

dcd549c

…thods (pythonGH-95240)

Co-authored-by: CAM Gerlach <[email protected]>.
(cherry picked from commit 5012bed)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

bedevere-bot removed the needs backport to 3.10 label

Jul 26, 2022

bedevere-bot commented Jul 26, 2022

Copy link

Copy Markdown

GH-95268 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland mentioned this pull request

Jul 26, 2022

Document all parameters of all sqlite3 functions/methods #95235

Closed

erlend-aasland added a commit that referenced this pull request

Jul 26, 2022


          [3.10] gh-95235: Add explicit parameter list to some sqlite3 methods (G…

a4fd732

…H-95240) (#95268)

Co-authored-by: CAM Gerlach <[email protected]>.
(cherry picked from commit 5012bed)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

ambv pushed a commit that referenced this pull request

Jul 26, 2022


          gh-95235: Add explicit parameter list to some sqlite3 methods (GH-95240…

5a9920f

…) (GH-95263)

Co-authored-by: CAM Gerlach <[email protected]>
(cherry picked from commit 5012bed)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

Reviewers

CAM-Gerlach

CAM-Gerlach approved these changes

berkerpeksag

Awaiting requested review from berkerpeksag

Uh oh!

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

Assignees

erlend-aasland

Labels

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.

4 participants

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.

GeezerVison

Conversation

erlend-aasland commented Jul 25, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Jul 25, 2022

Uh oh!

erlend-aasland commented Jul 25, 2022

Uh oh!

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 26, 2022

Uh oh!

miss-islington commented Jul 26, 2022

Uh oh!

bedevere-bot commented Jul 26, 2022

Uh oh!

bedevere-bot commented Jul 26, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erlend-aasland commented Jul 25, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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