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

Safe translations applied #1089

Merged
merged 16 commits into from
Jan 26, 2025
Merged

Safe translations applied #1089

merged 16 commits into from
Jan 26, 2025

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Jan 22, 2025

Fixes: #1088

How the translations work:

We have a key / value association for each language. English is the default and the reference one.
If a key is missing in another language, we want to fall-back to English. This fallback is a custom solution, Apple doesn't do this for us.

How it worked so far:

We were using the keys in the code, and applied the "payment.confirm.button.title".localized extension on it (our custom fallback function, or in some rare cases the "".localizedWithFormat(withArgs: ..) function, that can handle placeholders.

How could it go wrong:

What else can be (and now is) improved:

  • we have the qqq "special language", that contains the explanations for translators. Only that we do not really know which keys are really missing from there
  • we might have translation keys we do not use at all
  • we might use keys that were valid some time ago but no longer contained in the translation files

Solution:

Generate

  • in order to avoid mistyped keys, we can rely on the compiler itself. To do so, we generate a special swift file, that contains all the vars we can use, based on the English translation files. This gives us code-completion and compile time check.
  • we make sure that every key contains (at least one) dot '.'
  • we convert these into "_" for the swift variables eg: "payment.confirm.button.title" becomes:
    static let payment_confirm_button_title = "payment.confirm.button.title".localized
  • we make the .localized and the "".localizedWithFormat(withArgs: ..) function file private. This way it can not be used directly. This avoids the problem of eg. re-applying it twice on the same value, or using it on some other keys, the translation file doesn't know of.
  • we make this special swift file generated (at the same time we generate the project)
  • we add the generated file to .gitignore

Validate

  • we check if each key has a dot (".")
  • with the above, we can search all the swift files (apart from the generated one), and make sure that those keys are not used anywhere (directly), since the key itself contains the ".", while the generated swift var is not, a text search is enough for this purpose
  • we can print out all the missing "qqq" explanation entries (it's not an error, only info)
  • we can print out all the keys that are not used anywhere in the project (it's not an error, only info)
  • we add this validation step to the CI

Notes on additional changes

Therefore in light of the above, I have:

  • removed the unused translations
  • changed a few translation keys to contain "."
  • filled in all the qqq explanations, that were missing
  • found one more invalid key in the code, that I've replaced with a valid one
  • since the CI is linting (only) the changed files, I had to add some minor lint related fixes as well

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 20.95238% with 249 lines in your changes missing coverage. Please review.

Project coverage is 35.70%. Comparing base (d2cda4f) to head (e39238a).

Files with missing lines Patch % Lines
Views/Library/ZimFileDetail.swift 0.00% 40 Missing ⚠️
Views/Settings/Settings.swift 0.00% 37 Missing ⚠️
SwiftUI/Model/Enum.swift 22.22% 28 Missing ⚠️
Views/Settings/About.swift 0.00% 16 Missing ⚠️
Views/Library/ZimFilesCategories.swift 0.00% 8 Missing ⚠️
Views/SearchResults.swift 0.00% 8 Missing ⚠️
Views/Settings/LanguageSelector.swift 0.00% 7 Missing ⚠️
App/SidebarViewController.swift 0.00% 6 Missing ⚠️
SwiftUI/Model/DirectoryMonitor.swift 0.00% 6 Missing ⚠️
Views/Buttons/TabsManagerButton.swift 40.00% 6 Missing ⚠️
... and 35 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
- Coverage   37.70%   35.70%   -2.01%     
==========================================
  Files         123      132       +9     
  Lines        6598     7605    +1007     
==========================================
+ Hits         2488     2715     +227     
- Misses       4110     4890     +780     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kelson42
Copy link
Contributor

@BPerlakiH Here we need to generate an error in CI instead of just warnings if:

  • unused strings
  • missing translation string
  • undocumented string

@BPerlakiH
Copy link
Collaborator Author

@kelson42 , no problem, please see it updated.

@kelson42
Copy link
Contributor

@BPerlakiH Woukd like to merge it and will do it.... once CI passes :)

@BPerlakiH
Copy link
Collaborator Author

BPerlakiH commented Jan 26, 2025

@kelson42 now it seems to be good. There's still "flaky-ness" on the CI part (regardless of the changes in this PR) :(

@kelson42 kelson42 force-pushed the safe-translations-applied branch from 2f8fd6f to e39238a Compare January 26, 2025 09:51
@kelson42 kelson42 merged commit 42a29b2 into main Jan 26, 2025
7 checks passed
@kelson42 kelson42 deleted the safe-translations-applied branch January 26, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make translations safer
3 participants