Python: refactor scripts under scripts/startup/ to use str.format #120552

Campbell Barton · 2024-04-12T08:10:40+02:00

Campbell Barton commented

2024-04-12 08:10:40 +02:00

Part of modernizing scripts in Blender, where the previous convention
was to use percentage formatting which has become the "old" way to
format strings in Python.

See proposal for details #120453.

Part of modernizing scripts in Blender, where the previous convention was to use percentage formatting which has become the "old" way to format strings in Python. See proposal for details #120453.

Campbell Barton commented

2024-04-12 08:11:06 +02:00

@blender-bot build

Campbell Barton referenced this pull request

2024-04-12 08:12:10 +02:00

Proposal: use str.format instead of percentage formatting for core-scripts #120453

Campbell Barton requested review from Sybren A. Stüvel 2024-04-12 08:57:32 +02:00

Campbell Barton requested review from Bastien Montagne 2024-04-12 08:57:32 +02:00

Bastien Montagne approved these changes 2024-04-12 11:15:52 +02:00

Bastien Montagne left a comment

Obviously did not check every line, but looks in line with the design task.

Campbell Barton force-pushed pr-str-format-refactor from f53af30ed0 to 770d8e24a3

2024-04-12 13:10:06 +02:00

Compare

Campbell Barton force-pushed pr-str-format-refactor from 770d8e24a3 to f8448c3d31

2024-04-12 13:37:40 +02:00

Compare

Sybren A. Stüvel requested changes 2024-04-15 11:27:15 +02:00

Dismissed

Sybren A. Stüvel left a comment

I have a question about the replacement of %s with {:s}, like here:

- f.write("# Keying Set: %s\n" % ks.bl_idname)
+ f.write("# Keying Set: {:s}\n".format(ks.bl_idname))

Because %-formatting always needed a type indicator, %s had to be used. In this case, however, ks.bl_idname is already a string, and so the simpler {} is valid here too. What's the reason to go for the more complex {:s}? Personally I would only add extra formatting parameters when they are actually necessary, i.e. when there is the necessity to actually call str(…) on the formatted object. The format string language spec even mentions that :s is the default for string types and may be ommitted.

Diving into this further: when the formatted object is not a string, this format is not even valid, so it can cause errors:

>>> "{:s}".format(1.3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Unknown format code 's' for object of type 'float'
>>> "%s" % 1.3
'1.3'

As you can see, replacing %s with {:s} has to be done very carefully, and doing so would count as a functional change.

A similar argument can be made for replacing %d with {:d} by the way, when the parameter is already an integer. :d is the same as having no format specifier at all. And again, it is only valid on int whereas "%d" is also valid when formatting floats:

>>> "{:d}".format(1.3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Unknown format code 'd' for object of type 'float'
>>> "%d" % 1.3
'1'

I have a question about the replacement of `%s` with `{:s}`, like here: ```diff - f.write("# Keying Set: %s\n" % ks.bl_idname) + f.write("# Keying Set: {:s}\n".format(ks.bl_idname)) ``` Because `%`-formatting always needed a type indicator, `%s` had to be used. In this case, however, `ks.bl_idname` is already a string, and so the simpler `{}` is valid here too. What's the reason to go for the more complex `{:s}`? Personally I would only add extra formatting parameters when they are actually necessary, i.e. when there is the necessity to actually call `str(…)` on the formatted object. The [format string language spec](https://docs.python.org/3/library/string.html#grammar-token-format-spec-format_spec) even mentions that `:s` is the default for string types and may be ommitted. Diving into this further: when the formatted object is not a string, this format is not even valid, so it can cause errors: ```python >>> "{:s}".format(1.3) Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: Unknown format code 's' for object of type 'float' >>> "%s" % 1.3 '1.3' ``` As you can see, replacing `%s` with `{:s}` has to be done very carefully, and doing so would count as a functional change. A similar argument can be made for replacing `%d` with `{:d}` by the way, when the parameter is already an integer. `:d` is the same as having no format specifier at all. And again, it is only valid on `int` whereas `"%d"` is also valid when formatting `float`s: ```python >>> "{:d}".format(1.3) Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: Unknown format code 'd' for object of type 'float' >>> "%d" % 1.3 '1' ```

scripts/startup/bl_operators/anim.py Outdated

						
				@ -71,3 +70,2 @@

				        f.write("ks.use_insertkey_needed = %s\n" % ks.use_insertkey_needed)

				        f.write("ks.use_insertkey_visual = %s\n" % ks.use_insertkey_visual)

				        f.write("ks.use_insertkey_needed = {!r}\n".format(ks.use_insertkey_needed))

Sybren A. Stüvel commented

2024-04-15 11:10:55 +02:00

This changes the semantics of the format string, from calling str(…) to repr(…) on the formatted object. It might be more correct to use the latter when generating Python code, but for this PR I would strongly recommend restricting it strictly to non-functional changes.

This changes the semantics of the format string, from calling `str(…)` to `repr(…)` on the formatted object. It might be more correct to use the latter when generating Python code, but for this PR I would strongly recommend restricting it strictly to non-functional changes.

ideasman42 marked this conversation as resolved

Campbell Barton force-pushed pr-str-format-refactor from f8448c3d31 to 1c4eec9ed6

2024-04-15 13:21:57 +02:00

Compare

Campbell Barton commented

2024-04-15 13:50:11 +02:00

@dr.sybren

I have a question about the replacement of %s with {:s}, like here:
- f.write("# Keying Set: %s\n" % ks.bl_idname)
+ f.write("# Keying Set: {:s}\n".format(ks.bl_idname))
Because %-formatting always needed a type indicator, %s had to be used. In this case, however, ks.bl_idname is already a string, and so the simpler {} is valid here too. What's the reason to go for the more complex {:s}? Personally I would only add extra formatting parameters when they are actually necessary, i.e. when there is the necessity to actually call str(…) on the formatted object. The format string language spec even mentions that :s is the default for string types and may be ommitted.

Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to.

For example, reading the following formatting, I find the type specifiers help with clarity.

file_preset.write("item_sub_{:d} = {:s}.add()\n".format(level, rna_path_step))

Callers that need to convert to str(..) can always use {!s}, then it's clear when a string is passed in, or a value that needed to be converted into a string was passed in instead.

On balance I'd rather always use type specifiers, but I'm not that fussed.

@mont29 what do you think about this?

@dr.sybren > I have a question about the replacement of `%s` with `{:s}`, like here: > > ```diff > - f.write("# Keying Set: %s\n" % ks.bl_idname) > + f.write("# Keying Set: {:s}\n".format(ks.bl_idname)) > ``` > Because `%`-formatting always needed a type indicator, `%s` had to be used. In this case, however, `ks.bl_idname` is already a string, and so the simpler `{}` is valid here too. What's the reason to go for the more complex `{:s}`? Personally I would only add extra formatting parameters when they are actually necessary, i.e. when there is the necessity to actually call `str(…)` on the formatted object. The [format string language spec](https://docs.python.org/3/library/string.html#grammar-token-format-spec-format_spec) even mentions that `:s` is the default for string types and may be ommitted. Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to. For example, reading the following formatting, I find the type specifiers help with clarity. ``` file_preset.write("item_sub_{:d} = {:s}.add()\n".format(level, rna_path_step)) ``` Callers that need to convert to `str(..)` can always use `{!s}`, then it's clear when a string is passed in, or a value that needed to be converted into a string was passed in instead. ---- On balance I'd rather always use type specifiers, but I'm not _that_ fussed. @mont29 what do you think about this?

Bastien Montagne commented

2024-04-15 18:17:22 +02:00

I would also rather have explicit type specifiers here, it helps disambiguating the code, and catching early errors.

Indeed this does imply potential functional changes in this patch.

I would also rather have explicit type specifiers here, it helps disambiguating the code, and catching early errors. Indeed this does imply potential functional changes in this patch.

Campbell Barton force-pushed pr-str-format-refactor from 1c4eec9ed6 to d1224d57de

2024-04-16 04:01:36 +02:00

Compare

Campbell Barton commented

2024-04-16 04:06:21 +02:00

I've gone over the patch and double checked type specifiers were used correctly the vast majority are very straightforward (translated labels, files/URL's & data-paths).

There were some cases where exceptions and keying set ID's needed to be converted into strings, committed fix.
Committed some fixes to ANIM_OT_keying_set_export in main (2b7f46bc0b), rebased this patch.

I've gone over the patch and double checked type specifiers were used correctly the vast majority are very straightforward (translated labels, files/URL's & data-paths). ---- - There were some cases where exceptions and keying set ID's needed to be converted into strings, committed fix. - Committed some fixes to `ANIM_OT_keying_set_export` in `main` (2b7f46bc0bc68fec02baaf31dc3eb1460eb4e880), rebased this patch.

Campbell Barton requested review from Sybren A. Stüvel 2024-04-16 04:06:31 +02:00

Campbell Barton force-pushed pr-str-format-refactor from d1224d57de to 342c2872c5

2024-04-16 04:45:35 +02:00

Compare

Sybren A. Stüvel referenced this pull request

2024-04-22 14:22:24 +02:00

Proposal: use str.format instead of percentage formatting for core-scripts #120453

Sybren A. Stüvel commented

2024-04-22 14:27:54 +02:00

Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to.

On balance I'd rather always use type specifiers, but I'm not that fussed.

I agree that what you describe here is somewhat personal. Personally I would strongly prefer type annotations and the structural use of mypy for this. If you need string formatting arguments to check your types for you, or to clarify which variable is of which (expected) type, then I feel that the type checking is done at the wrong level, in the wrong bit of code.

What I was describing has little to do with personal preference, though. Replacing %s with {:s} is objectively a functional change. The former can be applied to any Python object, whereas the latter only to strings.

For sanity sake, this pull request should limit itself to a non-functional change to another formatting API. Once that change is done, the strengths of that new API can be utilised to improve the code. These two should not be intertwined into one big commit.

> Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to. > On balance I'd rather always use type specifiers, but I'm not _that_ fussed. I agree that what you describe here is somewhat personal. Personally I would **strongly** prefer type annotations and the structural use of mypy for this. If you need string formatting arguments to check your types for you, or to clarify which variable is of which (expected) type, then I feel that the type checking is done at the wrong level, in the wrong bit of code. What I was describing has little to do with personal preference, though. Replacing `%s` with `{:s}` is objectively a functional change. The former can be applied to any Python object, whereas the latter only to strings. For sanity sake, this pull request should limit itself to a non-functional change to another formatting API. Once that change is done, the strengths of that new API can be utilised to improve the code. These two should not be intertwined into one big commit.

Campbell Barton commented

2024-04-24 04:47:23 +02:00

Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to.

On balance I'd rather always use type specifiers, but I'm not that fussed.

I agree that what you describe here is somewhat personal. Personally I would strongly prefer type annotations and the structural use of mypy for this. If you need string formatting arguments to check your types for you, or to clarify which variable is of which (expected) type, then I feel that the type checking is done at the wrong level, in the wrong bit of code.

Proper support of mypy & it's type hints would be helpful but I don't see these as mutually exclusive, if you don't use type specifiers then the arguments to format(..) are effectively untyped, a change to a type for e.g. (which mypy would accept) could be result in unexpected formatting.

What I was describing has little to do with personal preference, though. Replacing %s with {:s} is objectively a functional change. The former can be applied to any Python object, whereas the latter only to strings.

For sanity sake, this pull request should limit itself to a non-functional change to another formatting API. Once that change is done, the strengths of that new API can be utilised to improve the code. These two should not be intertwined into one big commit.

It's disputable what is/isn't a functional change in a context when strings are clearly in use e.g.:

"A {:s} B".format(", ".join(args))
text="{:s} ▶ {:s}".format(iface_(domain_name), iface_(data_type.name))

Nevertheless, its a large patch and there is some chance converting to a string was needed, so I'll use {!s} for strings and switch to {:s} in a separate commit.

Note that there doesn't seem to be an equivalent %d, so in this cases I'll check each instance as there aren't so many.

> > Good points and this does come down to personal preference, there are times I read over code and it's not clear what the original developer intended, in most cases I would rather get an error message if something which is intended to be one type - (but got broken in a refactor or similar). Instead of working but producing a string which doesn't error but doesn't work as it's supposed to. > > > On balance I'd rather always use type specifiers, but I'm not _that_ fussed. > > I agree that what you describe here is somewhat personal. Personally I would **strongly** prefer type annotations and the structural use of mypy for this. If you need string formatting arguments to check your types for you, or to clarify which variable is of which (expected) type, then I feel that the type checking is done at the wrong level, in the wrong bit of code. Proper support of mypy & it's type hints would be helpful but I don't see these as mutually exclusive, if you don't use type specifiers then the arguments to `format(..)` are effectively untyped, a change to a type for e.g. (which mypy would accept) could be result in unexpected formatting. > What I was describing has little to do with personal preference, though. Replacing `%s` with `{:s}` is objectively a functional change. The former can be applied to any Python object, whereas the latter only to strings. > > For sanity sake, this pull request should limit itself to a non-functional change to another formatting API. Once that change is done, the strengths of that new API can be utilised to improve the code. These two should not be intertwined into one big commit. It's disputable what is/isn't a functional change in a context when strings are clearly in use e.g.: - `"A {:s} B".format(", ".join(args))` - `text="{:s} ▶ {:s}".format(iface_(domain_name), iface_(data_type.name))` Nevertheless, its a large patch and there is some chance converting to a string was needed, so I'll use `{!s}` for strings and switch to `{:s}` in a separate commit. Note that there doesn't seem to be an equivalent `%d`, so in this cases I'll check each instance as there aren't so many.

Campbell Barton force-pushed pr-str-format-refactor from 342c2872c5 to b47ff3031e

2024-04-24 05:04:16 +02:00

Compare

Campbell Barton force-pushed pr-str-format-refactor from b47ff3031e to 3a3a6257ee

2024-04-25 04:19:28 +02:00

Compare

Campbell Barton force-pushed pr-str-format-refactor from 3a3a6257ee to 86cc3316d7

2024-04-25 16:26:57 +02:00

Compare

Campbell Barton force-pushed pr-str-format-refactor from 86cc3316d7 to 742386f19f

2024-04-26 10:58:16 +02:00

Compare

Sybren A. Stüvel approved these changes 2024-04-26 15:43:04 +02:00

Sybren A. Stüvel left a comment

Proper support of mypy & it's type hints would be helpful but I don't see these as mutually exclusive

Oh for sure. And I also see the advantage of more explicit type indicators in the format string, especially when translating them.

Nevertheless, its a large patch and there is some chance converting to a string was needed, so I'll use {!s} for strings and switch to {:s} in a separate commit.

Thanks.

Note that there doesn't seem to be an equivalent %d, so in this cases I'll check each instance as there aren't so many.

👍

> Proper support of mypy & it's type hints would be helpful but I don't see these as mutually exclusive Oh for sure. And I also see the advantage of more explicit type indicators in the format string, especially when translating them. > Nevertheless, its a large patch and there is some chance converting to a string was needed, so I'll use `{!s}` for strings and switch to `{:s}` in a separate commit. Thanks. > Note that there doesn't seem to be an equivalent `%d`, so in this cases I'll check each instance as there aren't so many. :+1:

Campbell Barton referenced this issue from a commit

2024-04-27 08:07:42 +02:00

Refactor: scripts under scripts/startup/ to use `str.format`

Campbell Barton referenced this pull request

2024-04-27 09:46:14 +02:00

Proposal: use str.format instead of percentage formatting for core-scripts #120453

Campbell Barton manually merged commit 0e3b594edb into main

2024-04-27 09:48:48 +02:00

Sign in to join this conversation.

No reviewers