Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Oct 31, 2020
Merged

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

merged 5 commits into from
Oct 31, 2020

Conversation

erlend-aasland
Copy link
Contributor

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

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

Convert _sqlite3 module level functions, except connect, to Argument …
39df100 
…Clinic

Note: Moves sqlite3.adapt from microprotocols.c to module.c
@erlend-aasland
Copy link
Contributor Author

@vstinner, would you mind reviewing this?

@erlend-aasland
Copy link
Contributor Author

@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
Copy link
Member

cc @shihai1991

@erlend-aasland
Copy link
Contributor Author

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

@corona10
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

@corona10 corona10 left a comment

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 ;)

Modules/_sqlite/module.c Outdated Show resolved Hide resolved
Modules/_sqlite/module.c Outdated Show resolved Hide resolved
Modules/_sqlite/module.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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 Aasland and others added 3 commits October 28, 2020 21:04
@corona10
complete_statement() should also accept 'statement' as a keyword argu…
fab488b 
…ment

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@corona10
Ditto for enable_shared_cache()
7ba5f23 
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@corona10
Keep current argument naming
81fd61c 
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@erlend-aasland
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

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
Copy link
Member

@corona10 corona10 left a comment

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:master Oct 31, 2020
@erlend-aasland
Copy link
Contributor Author

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-aasland erlend-aasland mentioned this pull request Dec 30, 2020
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@adorilson
bpo-40956: Convert _sqlite3 module level functions to Argument Clinic (…
f852401 
…pythonGH-22484)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corona10 corona10 corona10 approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

@vstinner vstinner Awaiting 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