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

gh-95132: Correctly relay *args and **kwds from sqlite3.connect to factory #95146

Merged
merged 9 commits into from
Jul 23, 2022
Merged

gh-95132: Correctly relay *args and **kwds from sqlite3.connect to factory #95146

merged 9 commits into from
Jul 23, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 22, 2022 •
edited
Loading

This PR partially reverts gh-24421 (PR) and fixes the remaining concerns
given in gh-93044 (issue).

Fixes gh-95132

@erlend-aasland erlend-aasland mentioned this pull request Jul 22, 2022
Closed
@erlend-aasland erlend-aasland marked this pull request as ready for review July 22, 2022 20:13
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 22, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit b144689 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 22, 2022
@erlend-aasland
Copy link
Contributor Author

Triggering the bots to be 100% sure we don't break 3.11 again.

@erlend-aasland
Copy link
Contributor Author

I've queued a commit that further improves the factory tests. I'll wait until the bots are done before pushing it, though 🤖

@erlend-aasland erlend-aasland added the needs backport to 3.11 only security fixes label Jul 22, 2022
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 23, 2022 •
edited
Loading

I've queued a commit that further improves the factory tests. I'll wait until the bots are done before pushing it, though 🤖

Bot run: 87 successful and 1 skipped checks 💯

serhiy-storchaka
serhiy-storchaka approved these changes Jul 23, 2022
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, but <unrepresentable> makes the exact signature almost useless. And there is no need to use it, because the default value can be represented.

@erlend-aasland
Copy link
Contributor Author

LGTM, but <unrepresentable> makes the exact signature almost useless. And there is no need to use it, because the default value can be represented.

Thanks for reviewing! Yeah, that's a C&P artefact from the clinic generated docstring. I'll fix it. Thanks again!

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 23, 2022 •
edited
Loading

This PR also addresses the remaining concerns in #93044:

  1. Keyword arguments are passed as positional arguments to factory().
  2. If an argument is not passed to sqlite3.connect(), its default value is passed to factory().

@serhiy-storchaka
Copy link
Member

Both the second and the third concerns.

@serhiy-storchaka
Update Modules/_sqlite/module.c
ca41429 
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@erlend-aasland
Copy link
Contributor Author

Both the second and the third concerns.

Yes, I've already updated my post. Also thanks for the last got'cha.

@erlend-aasland erlend-aasland merged commit a3d4d15 into python:main Jul 23, 2022
@miss-islington
Copy link
Contributor

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

@erlend-aasland erlend-aasland deleted the sqlite-ac-connect branch July 23, 2022 07:51
@miss-islington
Copy link
Contributor

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

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Jul 23, 2022
@erlend-aasland
[3.11] pythongh-95132: Correctly relay *args and **kwds from sqlite3.…
dbee099 
…connect to factory (pythonGH-95146)

This PR partially reverts pythongh-24421 (PR) and fixes the remaining concerns
given in pythongh-93044 (issue):

- keyword arguments are passed as positional arguments to factory()
- if an argument is not passed to sqlite3.connect(), its default value
  is passed to factory()

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>.
(cherry picked from commit a3d4d15)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 23, 2022
@bedevere-bot
Copy link

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

@erlend-aasland erlend-aasland mentioned this pull request Jul 23, 2022
Closed
erlend-aasland pushed a commit that referenced this pull request Jul 23, 2022
[3.11] gh-95132: Correctly relay *args and **kwds from sqlite3.connec…
5d3f2fb 
…t to factory (GH-95146) (#95158)

This PR partially reverts gh-24421 (PR) and fixes the remaining concerns
given in gh-93044 (issue):

- keyword arguments are passed as positional arguments to factory()
- if an argument is not passed to sqlite3.connect(), its default value
  is passed to factory()

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>.
(cherry picked from commit a3d4d15)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@erlend-aasland erlend-aasland mentioned this pull request Aug 8, 2022
Merged
2 tasks
@AlexWaygood AlexWaygood mentioned this pull request Nov 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

Assignees

@erlend-aasland erlend-aasland

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python3.11: sqlite3.connect passes more arguments to factory()
4 participants
@erlend-aasland @bedevere-bot @serhiy-storchaka @miss-islington