Skip to content

bpo-40956: Convert _sqlite3.Cache to Argument Clinic #24135

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
Closed
wants to merge 1 commit into from
Closed

bpo-40956: Convert _sqlite3.Cache to Argument Clinic #24135

wants to merge 1 commit into from

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 6, 2021 •
edited by bedevere-bot
Loading

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 6, 2021

skip news

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 6, 2021

FYI, rebased onto master bco. #20828

@erlend-aasland erlend-aasland mentioned this pull request Jan 7, 2021
Closed
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 7, 2021 •
edited
Loading

@berkerpeksag What about ditching the whole Cache/Node stuff and just use functools.lru_cache? Pro: It would get rid of a lot of code. Con: It might be a little bit slower. Worth pursuing as a future enhancement?

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 10, 2021

@serhiy-storchaka Would you mind reviewing this?

@berkerpeksag
Copy link
Member

berkerpeksag commented Jan 11, 2021

@berkerpeksag What about ditching the whole Cache/Node stuff and just use functools.lru_cache?

I'm usually not a fan of changing code that has been working fine for more than a decade, but let me think about it a bit.

I'm going to review this PR this week.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 11, 2021 •
edited by bedevere-bot
Loading

I'm usually not a fan of changing code that has been working fine for more than a decade, but let me think about it a bit.

That's a valid argument, but take a look at my arguments at bpo-42862.

I'm going to review this PR this week.

Great, thanks.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 12, 2021

Thanks, @serhiy-storchaka 🙏🏻

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 12, 2021

Closing this, as it has been made obsolete by #24203.

@erlend-aasland erlend-aasland deleted the bpo-40956/part5-cache branch February 2, 2021 20:49
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

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland @berkerpeksag @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot