Skip to content

bpo-40956: Convert _sqlite3 module level functions to Argument Clinic - #22484

Merged
corona10 merged 5 commits into
python:masterfrom
erlend-aasland:bpo-40956/part1-mod
Oct 31, 2020
Merged

bpo-40956: Convert _sqlite3 module level functions to Argument Clinic#22484
corona10 merged 5 commits into
python:masterfrom
erlend-aasland:bpo-40956/part1-mod

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Oct 1, 2020•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Part 1 of 5, @vstinner.
Ref #20826

Notes:

  • _sqlite3.connect will be converted in the final PR
  • sqlite3.adapt is moved from microprotocols.c to module.c

https://bugs.python.org/issue40956

Erlend E. Aasland
Convert _sqlite3 module level functions, except connect, to Argument …
39df100
…Clinic
Note: Moves sqlite3.adapt from microprotocols.c to module.c
@erlend-aasland

erlend-aasland commented Oct 11, 2020

Copy link
Copy Markdown
ContributorAuthor

@vstinner, would you mind reviewing this?

@erlend-aasland

erlend-aasland commented Oct 15, 2020

Copy link
Copy Markdown
ContributorAuthor

@corona10 Would you you mind reviewing this as well, if you've got time for it? It will be a series of five (or six) PR's.

@corona10

corona10 commented Oct 20, 2020

Copy link
Copy Markdown
Member

cc @shihai1991

@erlend-aasland

erlend-aasland commented Oct 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Would it be preferable to split this PR in smaller chunks? One PR for each method converted?

@corona10

corona10 commented Oct 23, 2020

Copy link
Copy Markdown
Member

Would it be preferable to split this PR in smaller chunks? One PR for each method converted?

Oops sorry, I will review this PR until next week, sorry for the delay :(

@erlend-aasland

erlend-aasland commented Oct 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Oops sorry, I will review this PR until next week, sorry for the delay :(

No problem, @corona10, and thank you for taking this! I hope I'm not too eager/impatient.

corona10
corona10 requested changes Oct 28, 2020
View reviewed changes

@corona10corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked some APIs.
Please check other things also ;)

Comment threadModules/_sqlite/module.c Outdated
Comment threadModules/_sqlite/module.c Outdated
Comment threadModules/_sqlite/module.c Outdated
@bedevere-bot

bedevere-bot commented Oct 28, 2020

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Erlend Egeberg Aaslandand others added 3 commits October 28, 2020 21:04
Erlend Egeberg Aasland@corona10
complete_statement() should also accept 'statement' as a keyword argu…
fab488b
…ment
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Erlend Egeberg Aasland@corona10
Ditto for enable_shared_cache()
7ba5f23
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Erlend Egeberg Aasland@corona10
Keep current argument naming
81fd61c
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@erlend-aasland

erlend-aasland commented Oct 30, 2020

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Oct 30, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

corona10
corona10 approved these changes Oct 31, 2020
View reviewed changes

@corona10corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Thank you for working on this ;)

@corona10
corona10 merged commit 7d21027 into python:masterOct 31, 2020
@erlend-aasland

erlend-aasland commented Oct 31, 2020

Copy link
Copy Markdown
ContributorAuthor

Thank you for working on this ;)

Thank you and likewise! :)

@erlend-aasland
erlend-aasland deleted the bpo-40956/part1-mod branch October 31, 2020 09:41
@erlend-aaslanderlend-aasland mentioned this pull request Dec 30, 2020
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Erlend Egeberg Aasland@adorilson
bpo-40956: Convert _sqlite3 module level functions to Argument Clinic (…
f852401
…pythonGH-22484)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10corona10corona10 approved these changes
@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag
@vstinnervstinnerAwaiting requested review from vstinner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland@corona10@bedevere-bot@taleinat@the-knights-who-say-ni