GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44002: Switch to lru_cache in urllib.parse. by gpshead · Pull Request #25798 · python/cpython

Conversation

@gpshead

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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.
@bedevere-bot bedevere-bot added the awaiting core review label May 1, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 1, 2021
@gpshead gpshead added the DO-NOT-MERGE label May 1, 2021
@gpshead

gpshead commented May 1, 2021

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead requested a review from orsenthil May 1, 2021 22:55
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
rhettinger
rhettinger reviewed May 1, 2021
View reviewed changes
Comment thread Lib/test/test_urlparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
Use the default lru_cache size.
7397c2c
gpshead
gpshead commented May 2, 2021
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gpshead
Use a typed lru_cache on urlsplit.
8a54534
serhiy-storchaka
serhiy-storchaka reviewed May 2, 2021
View reviewed changes
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
Restore the list comprehensions.
7b70e99
@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.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
@gpshead gpshead removed the DO-NOT-MERGE label May 3, 2021
@gpshead gpshead self-assigned this May 3, 2021
@gpshead gpshead added the DO-NOT-MERGE label May 3, 2021
@gpshead

gpshead commented May 3, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
akulakov
akulakov reviewed May 11, 2021
View reviewed changes
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes May 11, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 11, 2021
gpshead added 2 commits May 11, 2021 15:48
@gpshead
Merge remote-tracking branch 'upstream/main' into non-90s-urlparse-cache
6c02506
@gpshead
Add a test for the module level __getattr__().
1129c9b
@gpshead gpshead added 🤖 automerge and removed DO-NOT-MERGE labels May 11, 2021
@miss-islington

miss-islington commented May 11, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented May 11, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead merged commit d597fdc into python:main May 12, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label May 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 :)
gpshead and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland mentioned this pull request May 12, 2021
Feature/replace list comp with generator #26046
Closed
@JelleZijlstra JelleZijlstra mentioned this pull request Nov 12, 2022
urllib: improve bytes handling python/typeshed#9167
Merged
@hugovk hugovk mentioned this pull request Jul 11, 2023
gh-105373: Doc lists pending removals #106540
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akulakov akulakov akulakov left review comments
@rhettinger rhettinger rhettinger left review comments
@tirkarthi tirkarthi tirkarthi left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@orsenthil orsenthil Awaiting requested review from orsenthil orsenthil is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@gpshead @rhettinger @orsenthil @miss-islington @akulakov @serhiy-storchaka @tirkarthi @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.