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

Experimental I18n Support (only client/common/inspector/interactive) #1

Closed
wants to merge 22 commits into from

Conversation

philtweir
Copy link
Owner

@philtweir philtweir commented Dec 29, 2024

Trial to see what would be involved in adding rust-i18n to support translatability. This only translates the crates atuin-common and atuin-client, plus inspector.rs and interactive.rs in the atuin crate.

I am pretty new to this, so keen for feedback from knowledgable folks. Rust internationalization does not seem to be in a greatly active state - most of the packages have long-standing issues/PRs without a lot of movement, and no one package provides a one-stop shop. sys-locale and rust-i18n are relatively well-used recently (2nd and 6th in lib.rs i18n package stats) and fairly uninvasive.

Using English & Irish and only the interactive search as a minimal test. To use:

LANG=C atuin search -i
LANG=ga atuin search -i

The main action points would be adding t!(...) everywhere appropriate, accounting for the non-static return values, and then progressively adding translations to app.yml files as people wish to PR.

Screenshot From 2024-12-29 20-53-27

Maybe overkill, but on the basis that every natural language string is a user-facing string, by the nature of a CLI tool, I have added t! basically everywhere. If a minimalist approach is better, easy to wind that back, but at least it is clear what the scale of changes is at the top end.

I have included one Chinese string (which would need confirmed by a native speaker!) to see how this works in a non-Latin charset in atuin-common but other than that, the examples are (English and) Irish.

Notes

  • the biggest logic impact is that, because the strings are dynamically composed based on the active locale, the compiler cannot check format strings where the translation string is parametrized, and that must be composed at runtime - it also means that conditionally-parsed format strings (like debug!) now have t! as a parameter instead, so the bulk of the string will be built regardless (this seems not a huge issue in an atuin context, but could be an argument for leaving them out of i18n). However, anywhere the static text can be split out from any parameters, I have done so, unless it seems possible that a translator might need to reorder the phrase in some language. E.g. "Loaded at: {time}" -> whole string parameterized, "Error in data: {err}" -> split and only "Error in data" translated
  • as t!(...) can take arguments (e.g. translate %{time} ago) it does not return &'static str so, for FilterMode, I used lazy_static and elsewhere, switched to String or otherwise worked-around.
  • for simplicitly, mostly I used the untranslated strings as the keys, but probably it would be better to invent slug-like keys
  • initialization of the locale happens in each crate, but other than pulling the macro in, only a single line is required to call atuin_common::utils::set_locale(), which consistently sets the locale for each crate
  • rust-i18n has two translation string file format versions - one splitting by language, and another all combined, so all languages are keyed under a given translation string -- I have taken the second, which is recommended for ease of translation (as the original and other translations are right beside). OTOH, having separate language files would avoid merge conflicts between languages
  • I didn't see much benefit having per-crate tests, especially as the logic is all in rust-i18n, but I did put a couple of sense-checking integration tests in atuin_common::utils where the set_locale and get_locale commands live
  • Project Fluent looks like a solid, standardized system, and is supported (to some extent) by Weblate but does not really look active given the size of project
  • the rust-i18n approach is really simple, but could be more standard to use gettext?

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@philtweir philtweir force-pushed the feature/i18n-support branch from 41bf685 to abbab77 Compare December 29, 2024 21:11
dependabot bot and others added 6 commits January 1, 2025 22:54
Bumps debian from bookworm-20241202-slim to bookworm-20241223-slim.

---
updated-dependencies:
- dependency-name: debian
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
These dependencies are unused in actual code, and the test I've removed
is a remnant from a move to use an external library -- it was useful to
show that the mechanical transformation was correct, but it's only
testing that library nowadays.
Use `if let` rather than `is_some()` followed by `unwrap()`, and coerce
errors instead of calling `unwrap()` when available.
* fix(wrapped): fix crash when history is empty

* style: fix format
* feat(wrapped): add more pkg managers

* style: fix format (missing comma)

* fix: remove pyenv as it doesn't handle packages
Bumps the cargo group with 2 updates: [tonic](https://github.com/hyperium/tonic) and [rsa](https://github.com/RustCrypto/RSA).


Updates `tonic` from 0.12.2 to 0.12.3
- [Release notes](https://github.com/hyperium/tonic/releases)
- [Changelog](https://github.com/hyperium/tonic/blob/master/CHANGELOG.md)
- [Commits](hyperium/tonic@v0.12.2...v0.12.3)

Updates `rsa` from 0.9.6 to 0.9.7
- [Changelog](https://github.com/RustCrypto/RSA/blob/v0.9.7/CHANGELOG.md)
- [Commits](RustCrypto/RSA@v0.9.6...v0.9.7)

---
updated-dependencies:
- dependency-name: tonic
  dependency-type: direct:production
  dependency-group: cargo
- dependency-name: rsa
  dependency-type: indirect
  dependency-group: cargo
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@philtweir philtweir changed the title I18n Support: Experiment Experimental I18n Support (only client/common/inspector/interactive) Jan 19, 2025
"%{time} ago":
en: "%{time} ago"
ga: "%{time} ó shin"
time.y:
Copy link
Owner Author

Choose a reason for hiding this comment

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

this is maybe excessive, as SI units are supposed to have the same representation in every language, but it gives an idea how this could work

Copy link
Owner Author

Choose a reason for hiding this comment

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

(and non-SI units like days, hours, minutes, etc. do vary)

@philtweir philtweir force-pushed the feature/i18n-support branch from 6245b6d to df7c1f8 Compare January 19, 2025 23:05
@philtweir
Copy link
Owner Author

Closing for atuinsh#2553

@philtweir philtweir closed this Jan 19, 2025
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.

3 participants