STYLE: Prefer using f-strings #198
Conversation
jhlegarreta
force-pushed
the
UsefStrings
branch
from
3bd0da7
to
389ee39
Compare
Prefer using f-strings.
jhlegarreta
force-pushed
the
UsefStrings
branch
from
389ee39
to
419a66b
Compare
There was a problem hiding this comment.
@jhlegarreta it would be nice to think about preparing SlicerDMRI for internationalization. I don't think we have a developer guide for this yet, but one aspect is that we are using .format instead of f-strings (e.g. like this). The reason is that if we used f-strings then translations of that string could introduce arbitrary code which would be a security issue.
Although I like f-strings generally, I think we should try to make the code ready for i18n.
Do you know whether the Python folks are investigating this pitfall on f-strings while being used for i18n? If they are, that would be excellent, and would give more value to this PR. Otherwise, TBH, it would be a very low priority task for me to use the On a slightly related note, the CI builds logs are full of traces related to internationalization, e.g. Seem to be warnings related to 3D Slicer or CTK, stemming from some Qt deprecation. It may be worthwhile to address those to avoid a visual pollution that prevents from identifying other warnings. And this happens despite telling CMake not to build 3D Slicer with i18n support: |
I have no idea. Probably the correct thing is to parse the f-string and only present the actual text for translation.
Can you point to a specific example or two? This log is huge and barely loads. The internationalization support is still new, so it's not a surprise that there are still some issues to resolve. |
If you allow sufficient time, the line posted will lead you directly to one of the messaged related to i18n. |
|
@lassoan @mhdiop do have ideas about the warnings that @jhlegarreta pointed to? |
|
We don't do anything special for i18n in Slicer, just use translation functions in a very simple, straightforward way. The build warnings are probably due to PythonQt and you can safely ignore it. PythonQt developers don't aim for warning-free code on all compilers. I would prefer if they tried a bit harder but it's an open-source library, so I guess anybody who is bothered could submit a pull request to fix warnings. You may try to use the latest Qt5 version, maybe it helps. Python's recommendation is to use named placeholders in translatable text. Python's core translation infrastructure (gettext) does not support f-strings at all. Our infrastructure could take f-strings, but it would be unsafe. Note that messages for developers must not be translated (it would make debugging much harder and unnecessarily increase translation workload), so you can keep using any string functions for messages that are only shown to developers. It could make sense to use |
Yes, the warnings look harmless, but they are polluting the logs and the builds in important ways, and prevent to navigate through them and to easily locate potentially harmful warnings. Even more so when we have instructed CMake not use i18n at all: https://github.com/SlicerDMRI/SlicerDMRI/blob/master/.github/workflows/build-test.yml#L65
I agree; a proper logging system should be in place. However, this is a low priority now. And of course, I do not aim at translating any logged message; i18n is even a low priority for the toolkit in its current state. |
|
The warning comes from Qt 5.15.2: Qt 5.12.11 still has it: The first release removing the warning is 6.0.0. Does 3D Slicer support Qt 6.0.x @lassoan @jcfr ? |
If you do a complete build: it helps if you review all the warnings once; and then only have look at the delta (new warnings). The dashboard shows new warnings separately (https://slicer.cdash.org/viewBuildError.php?type=1&onlydeltap&buildid=3224528), so it can be used. During development: you usually modify one or few files at a time, so seeing the extra warnings is annoying but should be manageable (at least it does not cause problems for me on Windows). If there are so many warnings that you cannot even see them easily even when modifying a single file then it may worth the effort of reducing them.
Qt6 must be supported first by PythonQt and CTK. Probably it will happen sometime in 2024. |
|
Well aware of all the above; the issue is inspecting the CI builds.
OK. Thanks. |
@lassoan Looks like there starts to be some alpha releases for Qt6 in PythonQt: |
|
Yes, thanks for the ping, it seems that @jamesobutler has already started looking into it. |
Prefer using f-strings.