More path cleanups, and ruff --fix#3587
Conversation
| assert f"app.paths.config={home / 'config' / full_name}" in results | ||
| assert f"app.paths.data={home / 'user_data' / full_name}" in results | ||
| assert f"app.paths.cache={home / 'cache' / full_name}" in results | ||
| assert f"app.paths.logs={home / 'logs' / full_name}" in results |
There was a problem hiding this comment.
Normally I'd opt for home / f"logs/{full_name}" over home / "logs" / full_name, to avoid creating an unnecessary intermediate Path object. However, in this case, I think it's more important to avoid nested f-strings when feasible.
(Also since these are all pulled off Path.home(), which is already absolute, I don't think there's any need for resolve(). Let me know if I'm missing something.)
There was a problem hiding this comment.
My recollection is the resolve() was needed because of how mounted network drives appear on Windows... but I might be misremembering that one. If it's passing CI, that's enough for me.
There was a problem hiding this comment.
For posterity:
There was a problem hiding this comment.
Huh. Any idea why that was needed, but removing it now doesn't reintroduce the error? Has handling of capitalization changed, either in pathlib or in Windows?
| rf"Unable to load icon from " | ||
| rf"{re.escape(str(app.paths.app / 'resources' / 'icons' / 'bad'))}" | ||
| r"Unable to load icon from " | ||
| + re.escape(str(app.paths.app / "resources/icons/bad")) |
There was a problem hiding this comment.
Maybe this is just me, but I hate f-strings that don't contain anything outside of the brackets. Sure, you get automatic concatenation with other string literals, but an operator's fine here. (It also converts to string, of course, but in this case re.escape() already returns one.)
There was a problem hiding this comment.
Yeah - this is a corner of Python syntax where there are no good answers. The default black/ruff answer of "split this string and don't even use an operator" is worse, IMHO, because easily mistaken for 2 separate arguments.
A moot point in this case, because you need the operator for the escape.
There was a problem hiding this comment.
FWIW, AFAIU, f-strings are optimized in cpython...especially with some shenanigans they pull; string addition still requires new memory allocations and memory copies. Not incredibly important....but why I look past it 🤷🏼
There was a problem hiding this comment.
Yeah - and in this case, it's not even f-strings that are the issue - the parser reads it as a single string, so there's only one allocation. A really minor optimisation, especially in this context, but an optimisation nonetheless.
There was a problem hiding this comment.
Turns out there is a good answer...
escaped_path = re.escape(str(app.paths.app / "resources/icons/bad"))
with pytest.raises(ValueError, match=rf"Unable to load icon from {escaped_path}"):
toga.Icon("resources/icons/bad")No nested f-strings, no more allocated strings than necessary (unless I'm mistaken), and it's fewer lines. 😎
There was a problem hiding this comment.
Unless the interpreter does some fancy optimization with strings that aren't assigned to variables, anyway. This and the original each create three strings (str, escape, and the f-string), but this one does assign one of them. (Not that it really matters here either way...)
There was a problem hiding this comment.
Not to drag this out too much farther... :) but this is what I mean. The new implementation introduces an additional operation in the bytecode to make one string at runtime. The original implementation had the parser create one string at compile time.
>>> import re
... def implicit(var1, var2):
... f"asdf asdf {var1}" f"{re.escape(var2)}"
... def addition(var1, var2):
... f"asdf asdf {var1}" + re.escape(var2)
...
>>>
>>> dis.dis(implicit)
2 RESUME 0
3 LOAD_CONST 1 ('asdf asdf ')
LOAD_FAST 0 (var1)
FORMAT_SIMPLE
LOAD_GLOBAL 0 (re)
LOAD_ATTR 3 (escape + NULL|self)
LOAD_FAST 1 (var2)
CALL 1
FORMAT_SIMPLE
BUILD_STRING 3
POP_TOP
RETURN_CONST 0 (None)
>>>
>>> dis.dis(addition)
4 RESUME 0
5 LOAD_CONST 1 ('asdf asdf ')
LOAD_FAST 0 (var1)
FORMAT_SIMPLE
BUILD_STRING 2
LOAD_GLOBAL 0 (re)
LOAD_ATTR 3 (escape + NULL|self)
LOAD_FAST 1 (var2)
CALL 1
BINARY_OP 0 (+)
POP_TOP
RETURN_CONST 0 (None)
>>>All that said, this may not even be faster....but there is at least a tangible difference in the two approaches.
There was a problem hiding this comment.
Yep! I understand, although thank you for the thoroughness. 😂
As of my most recent commit, there's no longer a + operator; the output of re.escape() is back inside an f-string (although it's assigned to a variable first).
| assert f"app.paths.config={home / 'config' / full_name}" in results | ||
| assert f"app.paths.data={home / 'user_data' / full_name}" in results | ||
| assert f"app.paths.cache={home / 'cache' / full_name}" in results | ||
| assert f"app.paths.logs={home / 'logs' / full_name}" in results |
There was a problem hiding this comment.
My recollection is the resolve() was needed because of how mounted network drives appear on Windows... but I might be misremembering that one. If it's passing CI, that's enough for me.
| rf"Unable to load icon from " | ||
| rf"{re.escape(str(app.paths.app / 'resources' / 'icons' / 'bad'))}" | ||
| r"Unable to load icon from " | ||
| + re.escape(str(app.paths.app / "resources/icons/bad")) |
There was a problem hiding this comment.
Yeah - this is a corner of Python syntax where there are no good answers. The default black/ruff answer of "split this string and don't even use an operator" is worse, IMHO, because easily mistaken for 2 separate arguments.
A moot point in this case, because you need the operator for the escape.
|
Ah - except for the merge conflict :-) |
[shakes fist] |
3 participants
Yet another pass at tidying / shortening paths. While doing so, I noticed that the Ruff pre-commit hook doesn't automatically run --fix, but it can be added as an argument, so I put that in here too.
PR Checklist: