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

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

Merged
merged 7 commits into from
Oct 6, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Nov 5, 2019

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

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.
It belongs with a later _ERR_MSG cleanup.
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

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

_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.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Need to decide what the ultimate fate of _ERR_MSG is.

@bedevere-bot
Copy link

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

@csabella
Copy link
Contributor

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

@ambv
Copy link
Contributor

ambv commented May 17, 2022

This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.

@ambv ambv removed the needs backport to 3.9 only security fixes label May 17, 2022
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.11 only security fixes label May 20, 2022
@gpshead gpshead removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 6, 2022
@gpshead
Copy link
Member Author

gpshead commented Oct 6, 2022

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 left _ERR_MSG and _ERR_MSG_PREFIX in place as they could be used by others. The code now uses the latter within its own f-string.

PTAL

Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

Other than the tiny nit I mentioned, LGTM.

@@ -1156,8 +1154,7 @@ def _find_and_load(name, import_):
_lock_unlock_module(name)

if module is None:
message = ('import of {} halted; '
'None in sys.modules'.format(name))
message = (f'import of {name} halted; None in sys.modules')
Copy link
Member

Choose a reason for hiding this comment

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

The parens here can be deleted; it looks like they were previously used for the multi-line string.

@gpshead gpshead merged commit 683ab85 into python:main Oct 6, 2022
@gpshead gpshead deleted the minor-importlib-opts branch October 6, 2022 23:43
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
* main:
  bpo-35540 dataclasses.asdict now supports defaultdict fields (pythongh-32056)
  pythonGH-91052: Add C API for watching dictionaries (pythonGH-31787)
  bpo-38693: Use f-strings instead of str.format() within importlib (python#17058)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
…thon#17058)

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.

We keep `_ERR_MSG` and `_ERR_MSG_PREFIX` as those may be used elsewhere for canonical looking error messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants