-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Internationalization (I18N) #1944
base: main
Are you sure you want to change the base?
Conversation
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Thanks @Goutte ! And to top it off, the translations are in French, which I am able to review 😀 |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this! Big feature add - I definitly want to be involved in the review of this since it's a rather sneaky breaking change (i.e., CLI apps that use their own internationalization, apps that expect output from errors of a certain kind, etc.)
But it's worth diving into!
I was quite a dive, but I was already in the lagoon and I wanted it for git spend (shameless plug), since having some english strings sprinkled in french doc was a tad uncanny. I reviewed this today, and I find the translation-as-a-function approach to be verbose, but explicit enough (and this way we get automatic edition of toml files). The dependencies are wrong, though, and desperately need some love. Suspects:
Thanks for the kind words, I'm glad you're considering adding I18N to cobra one way or another. |
Hi @Goutte - what's the status of this? This PR needs a rebase but it'd be a cool feature add to get this into the next release |
@jpmcb The I can rebase, but I think this MR needs more work to reduce its dependencies. I'm not sure how, though. |
Dependency HellI tried The thing is: I'm not about to code a lightweight alternative of the language shenanigans, in order to bypass these libs. Using init()We're setting up the localizer in an func init() {
setupLocalizer()
} As an alternative, perhaps I could try to lazily initialize whenever we use a |
Supports rfc4646 language tags in env vars. If region is detected (en-US, fr-CA), will try to load regional files first.
Flow is as follows: <add some new i18n.Message in code> make i18n_extract <fill the generated translations/translate.*.toml> make i18n_merge <perhaps delete translations/translate.*.toml> ?
This should keep backwards compat'.
Here's what I mean about the dependency bloat, see
Naughty stuff : |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Here's the
I looked at the most recent version of
|
Looking at where the
Nothing in And is language using anything that requires tools ?
Yes, once, but in a test file. Now, is go-i18n using anything else than Short answer : NO. ConclusionWe definitely DO NOT NEED THAT BLOAT. Perhaps I'm missing some magical tree shaking or something that would mitigate this issue ? After all, I'm still quite the noob in Go. 😝 Plan
I'm not about to do step 1 without some serious confirmation that I'm not horribly mistaken about all this. Even so, going against the core libs is … frightening, and not something I'm likely to do. |
The Again, I might be totally wrong. |
Might be replaced with #2090 |
How
ENV
(LANGUAGE
>LC_ALL
>LANG
)toml
translation filesBreaking
Pluralization
Output in English of some flag errors has changed a little, since pluralization is dynamic.
Eg:
accepts at most 2 args, received 3
andaccepts at most 1 arg, received 3
If that's not something you think we should have, and should stick with
arg(s)
to ease parsing for example (but… regex), I can revert this.go 1.16
go 1.16
is required forgo-i18n v2
. I enjoygoi18n extract
andmerge
too much to bother downgrading tov1
. Furthermore,go:embed
requiresgo 1.16
as well.Partial
Translations flow documentation
The translation flow is as follows:
Message
in a wrapper function inlocalizer.go
make i18n_extract
translations/translate.*.toml
filesmake i18n_merge
, it will write to thetranslations/active.*.toml
filestranslations/translate.*.toml
filesNot all strings translated
I focused on user-facing strings because those were the most useful to me.
I might do some more but probably won't do them all.
Bouerk
go.sum
went bananas, and I'm not sure what happened. Help appreciated. Some info here.