Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Fix all flake8-gettext (INT) errors (INT001, INT002, INT003) #4052

Merged
merged 23 commits into from
Sep 14, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jul 14, 2024

This fixes all instances in python files where translated strings are formatted before being fetched by gettext. (underscore function)
Ruff rules:
https://docs.astral.sh/ruff/rules/f-string-in-get-text-func-call/
https://docs.astral.sh/ruff/rules/format-in-get-text-func-call/
https://docs.astral.sh/ruff/rules/printf-in-get-text-func-call/

Some places I changed from percent formatting to dot format in order to use named parameters, to add more context for translators. Unfortunately, it might cause the source string to be different, and I'm not sure of the consequences on Weblate. From this FAQ https://docs.weblate.org/en/latest/faq.html#how-do-i-translate-several-branches-at-once, I understand that they don't really support translating for different branches, so having a source string changed makes it unavailable for backporting (if ever we are doing this). The translation files should be regenerated after this, as they are even out of date currently (merge conflicts when weblate tries to rebase because we use squash merge).

In a small number of cases, where I had added named format parameters, I also changed the strings around so they match the same string, and can be translated as one.

In r.in.wms, I'm pretty sure the variable "format" doesn't exist, and this, since the first commit of that file 12 years ago when it was migrated from an addon. I supposed it meant self.gdal_drv_format, from the context and another translated string near, but it is also possible it meant one of the parameters of the module (that wasn't called correctly if it was the case). Please take a look to confirm my guess.

These were all manual fixes, as no auto-fix was available.

I was committing in batches (also to help reviewing if it's too long in one go), I was planning on having the squash message clean this up.

@echoix echoix added this to the 8.5.0 milestone Jul 14, 2024
@echoix echoix requested a review from ninsbl July 14, 2024 17:20
@github-actions github-actions bot added GUI wxGUI related vector Related to vector data processing raster Related to raster data processing temporal Related to temporal data processing Python Related code is in Python libraries module general display imagery labels Jul 14, 2024
@echoix
Copy link
Member Author

echoix commented Jul 14, 2024

@neteler Maybe you have an opinion on this too

@neteler
Copy link
Member

neteler commented Jul 14, 2024

Unfortunately, it might cause the source string to be different, and I'm not sure of the consequences on Weblate.

I could imagine that all related existing translations would be changed to "fuzzy". If so, that might be very demotivating for the translators.

What does @HuidaeCho think?

@echoix
Copy link
Member Author

echoix commented Jul 14, 2024

If the translation files are up to date before this PR, there should be a way to undo changes to the source strings, recreate the translation files (I expect no changes), apply back the source string changes (possibly in small batches), recreate the translation files, and manually/with a tool combine back the changes to the translations.

@HuidaeCho
Copy link
Member

HuidaeCho commented Jul 17, 2024

Unfortunately, it might cause the source string to be different, and I'm not sure of the consequences on Weblate.

I could imagine that all related existing translations would be changed to "fuzzy". If so, that might be very demotivating for the translators.

What does @HuidaeCho think?

I'm not sure about that. These strings are still the same, so why would they become fuzzy? Because of line number changes? I'll try it when I get a chance.

@echoix
Copy link
Member Author

echoix commented Jul 17, 2024

Unfortunately, it might cause the source string to be different, and I'm not sure of the consequences on Weblate.

I could imagine that all related existing translations would be changed to "fuzzy". If so, that might be very demotivating for the translators.

What does @HuidaeCho think?

I'm not sure about that. These strings are still the same, so why would they become fuzzy? Because of line number changes? I'll try it when I get a chance.

That's the problem, some were changed just a little (placeholders), but maybe I could revert them.

@echoix
Copy link
Member Author

echoix commented Jul 17, 2024

Also, if you get emails about the status updates of our weblate, it got relocked this morning after a new PR merged...

gui/wxpython/iscatt/iscatt_core.py Show resolved Hide resolved
gui/wxpython/gmodeler/model.py Outdated Show resolved Hide resolved
gui/wxpython/gmodeler/panels.py Show resolved Hide resolved
gui/wxpython/gui_core/pyedit.py Show resolved Hide resolved
python/grass/temporal/univar_statistics.py Outdated Show resolved Hide resolved
gui/wxpython/gui_core/pyedit.py Show resolved Hide resolved
gui/wxpython/wxgui.py Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as it is except what is in the comments.

Just a heads up that keeping the messages same may have a similar frustration effect as suggested in a comment by @neteler just some time later. For example, string "Unable to remove timestamp for raster map" is correctly extracted for translation and if I understand correctly will therefore remain the same even after this PR. Because the string is no finally passed to the translate function (and not its modified version), it will be translated for the user for the first time only after this PR. Later, we may be changing all %-style strings or, much more likely, at least all strings with multiple %s or {} to format-style with keywords ({name}), so this will change the strings for sure. A string translated in the interface (thanks to this PR) will then be removed. So, same additional work for the translators as if we would change the strings now, but with more frustration with the final product where the once translated strings will disappear.

Overall, this PR (both the fixes and the enabled warning) is crucial for making the translation actually work consistently. The exact order of calls is clearly hard to grasp for contributors who don't deal with the translations themselves as demonstrated by the number of corrections needed here. This PR is fixing previous mistakes and avoids future ones.

python/grass/temporal/univar_statistics.py Outdated Show resolved Hide resolved
Co-authored-by: Vaclav Petras <[email protected]>
@echoix
Copy link
Member Author

echoix commented Sep 4, 2024

Clangformat has some activity on this error today

@echoix
Copy link
Member Author

echoix commented Sep 4, 2024

See DoozyX/clang-format-lint-action#75 (review) for the second PR supposed to fix that

gui/wxpython/gmodeler/model.py Outdated Show resolved Hide resolved
gui/wxpython/gmodeler/model.py Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
gui/wxpython/gmodeler/model.py Outdated Show resolved Hide resolved
python/grass/temporal/univar_statistics.py Show resolved Hide resolved
python/grass/temporal/univar_statistics.py Show resolved Hide resolved
wenzeslaus
wenzeslaus previously approved these changes Sep 13, 2024
@wenzeslaus wenzeslaus removed the request for review from ninsbl September 13, 2024 17:26
@echoix
Copy link
Member Author

echoix commented Sep 13, 2024

@wenzeslaus can you re-approve (or anyone)? I had to resolve a conflict in t.rast.what (a line was removed)

@echoix echoix merged commit 95da3ef into OSGeo:main Sep 14, 2024
27 checks passed
@echoix echoix deleted the fix-flake8-gettext-INT branch September 14, 2024 14:34
@echoix
Copy link
Member Author

echoix commented Sep 14, 2024

So now, we need to regenerate the template files. Last time it was @HuidaeCho that did it. Is it necessary for it to be a manual job?

Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
…SGeo#4052)

* style: Fix all flake8-gettext (INT) errors (INT001, INT002, INT003)

Ruff rules:
https://docs.astral.sh/ruff/rules/f-string-in-get-text-func-call/
https://docs.astral.sh/ruff/rules/format-in-get-text-func-call/
https://docs.astral.sh/ruff/rules/printf-in-get-text-func-call/

* Revert source string changes when unneeded.

* Update univar_statistics.py

Co-authored-by: Vaclav Petras <[email protected]>

* Update panels.py for Python script type string

* Update pyedit.py to keep path keyword in changed string

* Update univar_statistics.py to make already translated strings match again

* Update pyedit.py

* Update model.py

---------

Co-authored-by: Vaclav Petras <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display general GUI wxGUI related imagery libraries module Python Related code is in Python raster Related to raster data processing temporal Related to temporal data processing vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants