-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: Update old .format to f-string #30547
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
Conversation
Reviewed and updated to f-string for: pandas/compat/pickle_compat.py pandas/_config/config.py pandas/core/arrays/boolean.py Contributes to GH29547
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.
Couple of comments, other than that looks good. Thanks @thepaullee
pandas/_config/config.py
Outdated
|
||
for i, p in enumerate(path[:-1]): | ||
if not isinstance(cursor, dict): | ||
raise OptionError(msg.format(option=".".join(path[:i]))) | ||
raise OptionError( | ||
f"Path prefix to option '{'.'.join(path[:i])}' is already an option" |
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.
I think @jbrockmendel prefers to define expressions like this in a variable first, instead of using them directly in the f-string.
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.
yes i do. thanks for keeping an eye out
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.
my preference would be to leave the .format syntax when message used more than once to help ensure consistency of error messages.
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.
Reverted to the original .format syntax, but will keep the earlier comments in mind for the future!
pandas/core/arrays/boolean.py
Outdated
"'other' should be pandas.NA or a bool. Got {} instead.".format( | ||
type(other).__name__ | ||
) | ||
f"'other' should be pandas.NA or a bool.\ |
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.
We avoid backslashes in general, you can close the string here, and start it again in the next line, and they'll concatenate. Just leave a space at the end of this first string, and just make the second a f-string.
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.
Good to know, done!
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.
Minor thing, other than that lgtm. Thanks @thepaullee
Co-Authored-By: Marc Garcia <[email protected]>
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.
Thanks @thepaullee
Thanks @thepaullee |
Reviewed and updated to f-string for:
pandas/compat/pickle_compat.py
pandas/_config/config.py
pandas/core/arrays/boolean.py
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Marking pickle_compat.py as done but I left the existing
.format
as is, as I believe this is a good use case for.format