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-32262: Fix codestyle; use f-strings formatting where necessary. #4775

Merged
merged 5 commits into from Dec 10, 2017

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Dec 9, 2017

else:
handle = 'closed'
return '<%s %s>' % (self.__class__.__name__, handle)
return '<{} {}>'.format(self.__class__.__name__, handle)
Copy link
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
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.

if flags:
msg.append('flags=%r' % flags)
msg.append(f'flags={flags!r}')
msg = ', '.join(msg)
logger.debug('Get address info %s', msg)
Copy link
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
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
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
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.

return '<%s %s>' % (self.__class__.__name__, coro_repr)
coro_repr += f', created at {frame[0]}:{frame[1]}'

return '<{} {}>'.format(self.__class__.__name__, coro_repr)
Copy link
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
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!

logger.debug('%r communicate: feed stdin (%s bytes)',
self, len(input))
logger.debug(
'%r communicate: feed stdin (%s bytes)', self, len(input))
Copy link
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
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
if family:
msg.append('family=%r' % family)
msg.append(f'family={family!r}' % family)
Copy link
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
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.

None yet

6 participants