Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Convert all string concatenations and brace-formatting to f-string formatting #8332

Closed
ShadowJonathan opened this issue Sep 16, 2020 · 10 comments
Labels
z-p5 (Deprecated Label)

Comments

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Sep 16, 2020

Description:

If/when synapse drops support for 3.5, convert all brace-formatting ("{}".format(obj), percent-formatting ("%s" % obj) and string concatenations ("object:" + obj") with f-strings (f"{obj}")

This approach is cleaner and less bug-prone as variable reference and exacting string template is combined into one.

(When synapse drops support for 3.5, i'd be happy to take up the task of converting everything)

@richvdh
Copy link
Member

richvdh commented Jan 13, 2021

less bug-prone

is there actually much advantage to proactively going through and changing all the existing formatting operations? I'd have thought there was a much higher chance of introducing bugs by doing so.

@ShadowJonathan
Copy link
Contributor Author

less bug-prone

is there actually much advantage to proactively going through and changing all the existing formatting operations? I'd have thought there was a much higher chance of introducing bugs by doing so.

not if care is taken while going through them, and the "less bug-prone" part is in regards to how-to-use formatting in the future, because other formatting relies on careful ordering of the arguments in and out of the template, while f-strings have the actual referred variable right there in the template itself, which will by itself eliminate the possibility for misplaced variables

@richvdh
Copy link
Member

richvdh commented Jan 13, 2021

not if care is taken while going through them,

pretty sure that no matter how much care you take, changing code is more likely to introduce bugs than not doing so ;)

and the "less bug-prone" part is in regards to how-to-use formatting in the future,

well quite. that sounds like a good argument for using them for future code, whereas the best that can be said for changing existing code is "consistency". So sure, it's probably a good thing to do, but given the effort required, it doesn't feel like a massive priority.

@clokep clokep added the z-p5 (Deprecated Label) label Jan 13, 2021
@ShadowJonathan
Copy link
Contributor Author

Looking at https://github.com/asottile/pyupgrade, this seems easily done with pyupgrade --py36-plus, which'd also change a lot of other stuff to match pythonic python 3.6

@clokep
Copy link
Member

clokep commented Feb 11, 2021

A discrete downside to this is that, when using the logging module, it forces the evaluation of log messages, even if the message will not be logged (e.g. the logger's level isn't high enough).

@ShadowJonathan
Copy link
Contributor Author

A discrete downside to this is that, when using the logging module, it forces the evaluation of log messages, even if the message will not be logged (e.g. the logger's level isn't high enough).

@clokep what do you mean? logger.info("%s", variable)? pyupgrade doesn't touch that, it only converts "%s" % variable-esc formatting.

@clokep
Copy link
Member

clokep commented Feb 11, 2021

I don't know anything about pyupgrade, I'm just commenting on the title of this which says to convert formatting to f-strings.

@ShadowJonathan
Copy link
Contributor Author

I'm leaving alone logger.-calls, obviously, there are already many arguments from the community why they're simply "just useful" the way they are.

@clokep
Copy link
Member

clokep commented Feb 11, 2021

I'm leaving alone logger.-calls, obviously

I would not call this obvious from the description. 😄 There was no mention of leaving logger calls alone.

there are already many arguments from the community why they're simply "just useful" the way they are.

I'm glad that we're in agreement!

@ShadowJonathan
Copy link
Contributor Author

#9744 is already covering this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
z-p5 (Deprecated Label)
Projects
None yet
Development

No branches or pull requests

4 participants