Skip to content

bpo-32262: Fix codestyle; use f-strings formatting where necessary.#4775

Merged
1st1 merged 5 commits intopython:masterfrom
1st1:cleanup
Dec 10, 2017
Merged

bpo-32262: Fix codestyle; use f-strings formatting where necessary.#4775
1st1 merged 5 commits intopython:masterfrom
1st1:cleanup

Conversation

@1st1
Copy link
Copy Markdown
Member

@1st1 1st1 commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not f-string here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Was an oversight, fixed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

F-string here too please 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, that's logging :(

logger.debug('Get address info %s', msg) -- means that msg will be interpolated only if the logging is ON and the logging level is DEBUG. Otherwise, msg.__str__() won't be called. This is a performance thing, as __repr__() and __str__() can be expensive for some objects.

logger.debug(f'Get address info {msg}') would mean that msg.__str__() is always called, even when logging is disabled.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In this particular case we already computed the msg, so we can inline it. In 99.9% other cases we don't want to convert logging strings.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem is not only in performance.
Tools like sentry groups similar log messages basing on the message text. Calls with the same message template but different params are grouped together but pre-formatted templates are not.
As result the tool becames totally unusable: it displays tons of similar but unique messages.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be an f-string.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed. Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this can be made into f-string too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same

@1st1 1st1 merged commit 6370f34 into python:master Dec 10, 2017
msg = [f"{host}:{port!r}"]
if family:
msg.append('family=%r' % family)
msg.append(f'family={family!r}' % family)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's a remaining % family after the f-string

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, created a PR to fix this: #4787

Thanks!

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.

6 participants