Skip to content

bpo-44002: Switch to lru_cache in urllib.parse. - #25798

Merged
gpshead merged 7 commits into
python:mainfrom
gpshead:non-90s-urlparse-cache
May 12, 2021
Merged

bpo-44002: Switch to lru_cache in urllib.parse.#25798
gpshead merged 7 commits into
python:mainfrom
gpshead:non-90s-urlparse-cache

Conversation

@gpshead

@gpsheadgpshead commented May 1, 2021•
edited by miss-islington
Loading

Copy link
Copy Markdown
Member

urllib.parse now uses functool.lru_cache for its internal URL splitting and
quoting caches instead of rolling its own like its the 90s.

The undocumented internal Quoted class is now deprecated, for removal in 3.14.
No existing users were found.

clear_cache() is kept as twisted, gevent, and our own regression test runner were
found to be using it in their test suites.

https://bugs.python.org/issue44002

Automerge-Triggered-By: GH:gpshead

@gpshead
bpo-44002: Switch to lru_cache in urllib.parse.
630dd0a
urllib.parse now uses functool.lru_cache for its internal URL splitting and
quoting caches instead of rolling its own like its the 90s.
The undocumented internal Quoted class and clear_cache() APIs are now
deprecated, for removal in 3.14.
@gpshead

gpshead commented May 1, 2021

Copy link
Copy Markdown
MemberAuthor

I won't merge this until after the 3.10 branch is cut and all of the https://bugs.python.org/issue43882 fixes are in.

@gpshead
gpshead requested a review from orsenthilMay 1, 2021 22:55
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment threadLib/urllib/parse.py Outdated
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment threadLib/urllib/parse.py
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment threadLib/test/test_urlparse.py Outdated
gpshead
gpshead commented May 2, 2021
View reviewed changes

@gpsheadgpshead left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The test that's failing due to an unexpected BytesWarning in CI is annoying... that means we need to use lru_cache(typed=True) on the urlsplit decorator for safety to avoid other people who enable BytesWarning from seeing that warning.

serhiy-storchaka
serhiy-storchaka reviewed May 2, 2021
View reviewed changes
Comment threadLib/urllib/parse.py
@rhettinger

rhettinger commented May 2, 2021

Copy link
Copy Markdown
Contributor

It looks like the Travis CI failed only because of excess whitespace. Running 'make patchcheck' would likely clear it.

@orsenthil

orsenthil commented May 3, 2021

Copy link
Copy Markdown
Member

This final change, which addressed the review comments from the reviewers looks good to me.

@gpshead
don't deprecate clear_cache().
c2dbd84
twisted, gevent, and our own libregrtest use it in test suites. We'll
keep it for them.
... and skip Pending on Quoter's DeprecationWarning. No uses of that have
been found externally.
Also reoragnizes the unittest a bit to be cleaner and fixes a whitespace
problem.
@gpsheadgpshead self-assigned this May 3, 2021
@gpshead

gpshead commented May 3, 2021

Copy link
Copy Markdown
MemberAuthor

Status: awaiting a copy of #25853 to be merged into main before merging this one.

akulakov
akulakov reviewed May 11, 2021
View reviewed changes
Comment threadLib/urllib/parse.py
@miss-islington

miss-islington commented May 11, 2021

Copy link
Copy Markdown
Contributor

@gpshead: Status check is done, and it's a failure ❌ .

@miss-islington

miss-islington commented May 11, 2021

Copy link
Copy Markdown
Contributor

@gpshead: Status check is done, and it's a success ❌ .

@gpshead
gpshead merged commit d597fdc into python:mainMay 12, 2021
@gpshead
gpshead deleted the non-90s-urlparse-cache branch May 12, 2021 00:01
@orsenthil

orsenthil commented May 12, 2021

Copy link
Copy Markdown
Member

Excellent change. @gpshead :)

@erlend-aaslanderlend-aasland mentioned this pull request May 12, 2021
Closed
@JelleZijlstraJelleZijlstra mentioned this pull request Nov 12, 2022
Merged
@hugovkhugovk mentioned this pull request Jul 11, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akulakovakulakovakulakov left review comments
@rhettingerrhettingerrhettinger left review comments
@tirkarthitirkarthitirkarthi left review comments
@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes
@orsenthilorsenthilAwaiting requested review from orsenthil

Assignees

@gpsheadgpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@gpshead@rhettinger@orsenthil@miss-islington@akulakov@serhiy-storchaka@tirkarthi@the-knights-who-say-ni@bedevere-bot