Skip to content

bpo-43269: Clean up sqlite3 file scope - #24578

Merged
berkerpeksag merged 8 commits into
python:masterfrom
erlend-aasland:sqlite-scope
Feb 21, 2021
Merged

bpo-43269: Clean up sqlite3 file scope#24578
berkerpeksag merged 8 commits into
python:masterfrom
erlend-aasland:sqlite-scope

Conversation

@erlend-aasland

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

Copy link
Copy Markdown
Contributor

@erlend-aasland

erlend-aasland commented Feb 19, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Ref. #24569 (comment)

skip news

serhiy-storchaka
serhiy-storchaka reviewed Feb 19, 2021
View reviewed changes

@serhiy-storchakaserhiy-storchaka 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.

Are there redundant declarations in header files?

Comment threadModules/_sqlite/cache.c Outdated
@erlend-aasland

erlend-aasland commented Feb 19, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Are there redundant declarations in header files?

I already cleaned up those in a previous PR/issue (#20828). I'd like to get rid of the forward declarations in the top of the c files though, but that'll require moving a couple of the functions around.

berkerpeksag
berkerpeksag requested changes Feb 20, 2021
View reviewed changes

@berkerpeksagberkerpeksag 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.

This PR could be much shorter and easy to review without all of these noisy changes.

Comment threadModules/_sqlite/cache.c Outdated
Comment threadModules/_sqlite/cache.c Outdated
Comment threadModules/_sqlite/connection.c Outdated
Comment threadModules/_sqlite/connection.c Outdated
Comment threadModules/_sqlite/connection.c Outdated
Comment threadModules/_sqlite/statement.c Outdated
Comment threadModules/_sqlite/statement.c Outdated
Comment threadModules/_sqlite/util.c Outdated
Comment threadModules/_sqlite/util.c Outdated
Comment threadModules/_sqlite/util.c Outdated
@bedevere-bot

bedevere-bot commented Feb 20, 2021

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-aasland

erlend-aasland commented Feb 20, 2021

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Feb 20, 2021

Copy link
Copy Markdown

Thanks for making the requested changes!

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

berkerpeksag
berkerpeksag approved these changes Feb 21, 2021
View reviewed changes

@berkerpeksagberkerpeksag 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.

Much cleaner now, thank you!

@berkerpeksag
berkerpeksag merged commit bf838a6 into python:masterFeb 21, 2021
@erlend-aasland
erlend-aasland deleted the sqlite-scope branch February 21, 2021 06:09
@erlend-aasland

erlend-aasland commented Feb 21, 2021

Copy link
Copy Markdown
ContributorAuthor

Much cleaner now, thank you!

Thanks, Berker. After thinking about it, I do agree with you.

@erlend-aaslanderlend-aasland mentioned this pull request Feb 21, 2021
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Erlend Egeberg Aasland@adorilson
bpo-43269: Clean up sqlite3 file scope (pythonGH-24578)
c10eb82
@erlend-aaslanderlend-aasland mentioned this pull request Oct 26, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksagberkerpeksagberkerpeksag approved these changes
@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

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