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-43290: Remove pre SQLite 3.5.3 support code from pysqlite_step() #24638

Merged
merged 2 commits into from
Feb 25, 2021
Merged

bpo-43290: Remove pre SQLite 3.5.3 support code from pysqlite_step() #24638

merged 2 commits into from
Feb 25, 2021

Conversation

erlend-aasland
Copy link
Contributor

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

From the SQLite 3.5.3 changelog:

  • sqlite3_step() returns SQLITE_MISUSE instead of crashing when called
    with a NULL parameter.

https://bugs.python.org/issue43290

bpo-43290: Remove pre SQLite 3.5.3 support code from pysqlite_step()
776e0d7 
From the SQLite 3.5.3 changelog:
- sqlite3_step() returns SQLITE_MISUSE instead of crashing when called
  with a NULL parameter.
@erlend-aasland
Copy link
Contributor Author

@berkerpeksag or @serhiy-storchaka, would one of you mind reviewing this?

I guess skip news is ok here as well.

corona10
corona10 previously approved these changes Feb 24, 2021
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 workaround can be removed ;)

@erlend-aasland
Copy link
Contributor Author

lgtm workaround can be removed ;)

Thank you for reviewing, @corona10 :)

@corona10
Copy link
Member

Thank you for reviewing, @corona10 :)

But I left a comment about this issue, please reply it. Thank you

@corona10 corona10 dismissed their stale review February 24, 2021 15:16

Wait until disussion

@erlend-aasland
Copy link
Contributor Author

But I left a comment about this issue, please reply it. Thank you

I left 11 comments :)

berkerpeksag
berkerpeksag reviewed Feb 25, 2021
View reviewed changes
Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

I haven't read all of the comments (IMO too much discussion for a trivial detail), but looking at the original commit (ghaering/pysqlite@61b3cd9) and the SQLİte changelog, this looks pretty good to me. My only comment is about the superfluous comment.

Modules/_sqlite/util.c Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

I haven't read all of the comments (IMO too much discussion for a trivial detail), but looking at the original commit (ghaering/pysqlite@61b3cd9) and the SQLİte changelog, this looks pretty good to me. My only comment is about the superfluous comment.

I'll keep it shorter next time. Most of it was just a note to self about what was going on.

I'll remove the comment.

@erlend-aasland
Copy link
Contributor Author

PTAL, @berkerpeksag

berkerpeksag
berkerpeksag approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@berkerpeksag berkerpeksag 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! Will merge once everything is green.

@erlend-aasland
Copy link
Contributor Author

LGTM, thank you! Will merge once everything is green.

Thank you, @berkerpeksag, I appreciate it.

@berkerpeksag berkerpeksag merged commit 91ea37c into python:master Feb 25, 2021
@erlend-aasland erlend-aasland deleted the sqlite-cleanup-step branch February 26, 2021 06:28
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@adorilson
bpo-43290: Remove workaround from pysqlite_step() (pythonGH-24638)
32ebc1a 
From the SQLite 3.5.3 changelog:

sqlite3_step() returns SQLITE_MISUSE instead of crashing when called
with a NULL parameter.

The workaround no longer needed because we no longer support
SQLite releases older than 3.7.15.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

@corona10 corona10 corona10 left review comments

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 @berkerpeksag @the-knights-who-say-ni @bedevere-bot