Skip to content
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.

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

bpo-38693: Use f-strings instead of str.format() within importlib #17058

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

bpo-38693: Use f-strings instead of str.format() within importlib #17058

wants to merge 3 commits into from
+1,463 −1,472

Conversation

@gpshead
Copy link
Member

@gpshead gpshead commented Nov 5, 2019 •
edited by bedevere-bot

This is a small performance improvement, especially for one or two hot
places such as _handle_fromlist() that are called a lot and the
str.format() method was being used just to join two strings with a '.'.

Otherwise it is merely a readability improvement.

This could be backported to 3.8 as it does not change any logic.

I kept _ERR_MSG as an attribute in importlib._bootstrap and imp
as I wasn't sure if there were other things in the world that might
refer to those. They're private and could go away in 3.9 but should
not within 3.8 just in case.

https://bugs.python.org/issue38693
gpshead added 2 commits Nov 5, 2019
@gpshead
Use of f-strings instead of .format() in importlib.
5ffb681 
This is a small performance improvement, especially for one or two hot
places such as _handle_fromlist() that are called a lot and the
.format() method was being used just to join two strings with a dot.

Otherwise it is merely a readability improvement.

This could be backported to 3.8 as it changes no functionality.

I kept `_ERR_MSG` as an attribute in `importlib._bootstrap` and `imp`
as I wasn't sure if there were other things in the world that might
refer to those.  They're private and could go away in 3.9 but should
not within 3.8 just in case.
@gpshead
NEWS entry!
bcb2891
@gpshead gpshead requested a review from brettcannon Nov 5, 2019
@gpshead
undo the Lib/imp.py change.
592a529 
It belongs with a later _ERR_MSG cleanup.
ericvsmith
ericvsmith reviewed Nov 5, 2019
View changes
Copy link
Member

@ericvsmith ericvsmith left a comment

I think this is generally a good idea, and all of the changes look correct.

However, I'm not sure I'd remove the use of _ERR_MSG, since it is used outside of this file. Either that, or remove it in Lib/imp.py, too (the only other place it's used).

I'm also not sure what value is added by _ERR_MSG_PREFIX. It doesn't seem to be used anywhere. I'd just roll it into _ERR_MSG, if it's being kept.

@brettcannon
Copy link
Member

@brettcannon brettcannon commented Nov 5, 2019

_ERR_MSG was actually created specifically because there have been numerous instances where that sentence has been formatted differently across the code base (with or without a period, capitalized or not, wording, etc.). I think _ERR_MSG_PREFIX was created because it was used from import.c.

brettcannon
brettcannon requested changes Nov 5, 2019
View changes
Copy link
Member

@brettcannon brettcannon left a comment

Need to decide what the ultimate fate of _ERR_MSG is.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 5, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@csabella
Copy link
Contributor

@csabella csabella commented Jan 25, 2020

@gpshead, please take a look at the review comments. Thanks!

jaraco
jaraco approved these changes May 26, 2021
View changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith

@brettcannon brettcannon

@jaraco jaraco

Assignees
No one assigned
Labels
awaiting merge CLA signed needs backport to 3.9 needs backport to 3.10
Projects
None yet
Milestone
No milestone
7 participants
@gpshead @brettcannon @bedevere-bot @csabella @jaraco @ericvsmith @the-knights-who-say-ni