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-33440: Defer imports in pathlib to reduce its import time. by agfor · Pull Request #6820 · python/cpython

Conversation

@agfor

@agfor agfor commented May 14, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

https://bugs.python.org/issue33440

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@agfor
bpo-33440: Defer imports in pathlib to reduce its import time.
45f5e9b
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 14, 2018
@bedevere-bot bedevere-bot added the awaiting review label May 14, 2018
serhiy-storchaka
serhiy-storchaka reviewed May 14, 2018
View reviewed changes
Comment thread Lib/pathlib.py Outdated

def make_uri(self, path):
# Under Windows, file URIs use the UTF-8 encoding.
from urllib.parse import quote_from_bytes as urlquote_from_bytes

@serhiy-storchaka serhiy-storchaka May 14, 2018

Copy link
Copy Markdown
Member

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.

What is the reason of the renaming?

Sorry, something went wrong.

Uh oh!

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

@matrixise matrixise May 14, 2018

Copy link
Copy Markdown
Member

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.

there is no reason, you are right but this is just a copy. Now he could remove the renaming.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@agfor
bpo-33440: Remove unneeded import renames.
01d7fd4
matrixise
matrixise approved these changes May 14, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels May 14, 2018
@matrixise

matrixise commented May 14, 2018

Copy link
Copy Markdown
Member

@serhiy-storchaka I can't merge this PR, if you want to merge it, let's go! ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented May 14, 2018

Copy link
Copy Markdown
Member

I don't know whether this change is worth merging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka requested a review from pitrou May 14, 2018 19:52
@matrixise

matrixise commented May 14, 2018

Copy link
Copy Markdown
Member

ok thanks for your feedback @serhiy-storchaka
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella
csabella requested a review from ncoghlan May 22, 2019 10:19
@csabella
csabella requested review from ncoghlan and pitrou and removed request for ncoghlan and pitrou February 7, 2020 11:28
pitrou
pitrou requested changes Feb 7, 2020
View reviewed changes

@pitrou pitrou left a comment •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.

Sorry, it looks like this review took a long delay for a simple change. This looks good to me on the principle but you need to do two things:

  • merge / rebase your changes on git master
  • add a NEWS entry for the change (you can use blurb-it or blurb)

Also, would be nice if you posted import benchmark numbers for the change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Feb 7, 2020
@bedevere-bot

bedevere-bot commented Feb 7, 2020

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

Sorry, something went wrong.

Uh oh!

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

csabella commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

@agfor, please take a look at the review comments from pitrou. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood added the performance Performance or resource usage label Apr 11, 2022
@AlexWaygood

AlexWaygood commented Apr 11, 2022

Copy link
Copy Markdown
Member

I'm closing this PR, since it has been 2 years and @pitrou's review comments have still not been addressed.

@agfor: if you're still interested in working on this issue, please feel free to continue discussing it at #77621 and/or submit a new PR 🙂
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood closed this Apr 11, 2022
@ncoghlan ncoghlan mentioned this pull request Jun 21, 2022
Possible lazy import opportunities in pathlib #77621
Closed
@iritkatriel iritkatriel added 3.12 only security fixes and removed 3.12 only security fixes labels Sep 7, 2022
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 left review comments
@pitrou pitrou pitrou requested changes
@ncoghlan ncoghlan Awaiting requested review from ncoghlan
+1 more reviewer
@matrixise matrixise matrixise approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

awaiting changes performance Performance or resource usage

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

@agfor @matrixise @serhiy-storchaka @bedevere-bot @csabella @AlexWaygood @pitrou @iritkatriel @the-knights-who-say-ni
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.