Endilll wrote:
I think it's clear that we agree that:
1. Performance is irrelevant (and is not necessarily an argument in favor of
f-strings in the first place).
2. `!r` is obscure enough that we don't want to use it.
I also agree that we should move off printf-like formatting.
> cases that necessitate excessive or hard-to-read escaping (e.g.
> f"{{'{self.spelling} around line 3070) should be avoided: in those cases
> str.format() (possibly with named placeholders) are easier to read imo.
I don't think `str.format()` would help here, because it also requires to
escape curly braces. Only C-style formatting and string concatenation work with
singular curly braces.
While I've going over the changes once again, I found myself missing a dot in
the format string here:
<img width="1800" height="194" alt="image"
src="https://github.com/user-attachments/assets/98157f0f-270d-4ee3-b526-9f0d7ea734c1";
/>
Which to me corroborates the point that there is degradation in readability
while using f-strings.
> I also think that putting the values to be interpolated directly in the
> string, rather than at the end, is easier to read.
Even in a simpler cases like ours, where we rarely have more than three values
to put into a string?
I still think we should be going for `str.format()`, because most of our usages
have only few placeholders. When readability is a concern, placeholders with
short names should be used. Specifically, list expression on line 3190 would
benefit from a named placeholder, because at the moment it's not clear in the
review interface what data is put into the string (neither in the current code,
nor in the proposed changes), which is a mild maintenance concern.
As for line 3071, ideally I'd like something along the lines of
```python
"'{}', {}".format(self.spelling, self.kind).wrap_in("{", "}")
```
but in the absence of that (at least I'm not aware of this being available out
of the box),
```python
"{" + "'{}', {}".format(self.spelling, self.kind) + "}"
```
is fine by me.
https://github.com/llvm/llvm-project/pull/173861
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
