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

Implement localization using gettext files — I18N — L10N #2090

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Goutte
Copy link

@Goutte Goutte commented Dec 14, 2023

See #1134

Features

  • Recipe to extract new translations from the Go code: make i18n_extract
  • Using widely supported gettext format
  • Embedded lightweight MO files
  • Detect language from environment variables
  • Some strings were pluralized

Issues

No regional fallback for now, waiting for leonelquinteros/gotext#85 to be merged.

This will also unlock fallback to english when a translation is not found.


Replaces #1944

Copy link

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.

Copy link

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.

- Recipe to extract new translations from the Go code: `make i18n_extract`
- Embedded `MO` files
- Detect language from environment variables
- Some strings were pluralized
Copy link

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.

@Goutte
Copy link
Author

Goutte commented Dec 16, 2023

I know it's a lot of added lines, but more than half are from the POT and PO files. I added the french translation as an example, for good measure.

@marckhouzam marckhouzam added kind/feature A feature request for cobra; new or enhanced behavior area/cobra-command Core `cobra.Command` implementations labels Dec 22, 2023
@marckhouzam
Copy link
Collaborator

Thanks @Goutte!

A couple of things to help get this reviewed:
1- can resolve the conflicts with main
2- can you add in the description how to test this (including the best way to install xgotext)

@marckhouzam
Copy link
Collaborator

@Goutte could you explain what the process would be, if we adopted this PR, for a cobra contributor to add a new string printout? It is not clear to me when make i18n_extract should be run and which *.po/default.pot/*.mo files need to be modified by hand and when, etc.

At this point, clarifying these steps in the PR description is sufficient, but if the review goes well, we'll need those details in some cobra documentation.

Also, I'm also interested in understanding how a project using Cobra would proceed to add translations, or is that completely up to the project and this PR does not help?

@marckhouzam
Copy link
Collaborator

@Goutte could you explain what the process would be, if we adopted this PR, for a cobra contributor to add a new string printout? It is not clear to me when make i18n_extract should be run and which *.po/default.pot/*.mo files need to be modified by hand and when, etc.

I see this already in the docs. Nice work.
Can you clarify what you mean by "Make sure your software has also updated the MO files"?
Do we absolutely need to use a special editor like poedit or can this be done from the command-line?

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Just started looking at this but I'm staring to get it.
Seems good at first impression.

But, I feel we absolutely need a way to generate the *.mo files from the command-line, through the makefile in fact.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -33,3 +33,7 @@ install_deps:

clean:
rm -rf $(BIN)

i18n_extract:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do I understand correctly that whenever a file changes, this make target must be re-run to update the line numbers in the default.pot file?

If that is the case, then it is essential that we run this make target in .github/test.yml and check if default.pot (or anything else) gets changed. I don't expect many contributors to know they have to run this, so CI should fail if they don't

localizer.go Show resolved Hide resolved
@@ -33,15 +34,15 @@ func legacyArgs(cmd *Command, args []string) error {

// root command with subcommands, do subcommand checking.
if !cmd.HasParent() && len(args) > 0 {
return fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))
return fmt.Errorf(gotext.Get("LegacyArgsValidationError"), args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))
Copy link
Collaborator

@marckhouzam marckhouzam Dec 22, 2023

Choose a reason for hiding this comment

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

How about leaving the original english string as the index for all the gotext.Get() calls?
It would make reading the code much easier and it would make working with the *.po files easier as the original text would be right there as the index.

If the % formatting gives a problem, maybe we can replace it with %% in the index? It is not ideal, but it would be manageable. So, for this line here we would instead use (unless there is a better way?)

return fmt.Errorf(gotext.Get("unknown command %%q for %%q%%s"), args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))

Copy link
Collaborator

@marckhouzam marckhouzam Dec 23, 2023

Choose a reason for hiding this comment

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

I’ve looked at gotext in more detail, and the % form will work just fine as long as the parameters are in the gotext.Get() call instead of outside.

Copy link
Author

Choose a reason for hiding this comment

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

This is the crux of the matter.

I've done a whole bunch of implementations of translations in the past, and every time I try to use plain english instead of keys I end up either regretting it or refactoring heavily to keys.

Here's some food for thought when using raw english as keys:

  • cosmetically changing the english string (typo, whitespace, etc.) invalidates all the existing translations (possibly hundreds of files need to be updated, for nothing)
  • compels usage of context shenanigans for strings that are the same in english in different contexts but different in other languages
  • too many gettext parsers out there choke on nasty translation ids like the "quoted q'tara" <tag>
  • sometimes it creates a bias towards english in the structure of strings, that need to be fixed by translators, straight in the code, once again invalidating all existing translations

I'm very aware that using keys makes the code harder to read and understand. This is mitigated a little by a careful choice of the wording of the key.

I had to pick one way or the other ; it was not an easy choice, but it's one I made many times and I decided to go with hindsight from past experiences.

I'm not adamant on this, quite the contrary.
I listed some of the key points of my decision above (probably forgot some) ; I'll let y'all be the final judges. Good luck !

args.go Outdated
@@ -16,6 +16,7 @@ package cobra

import (
"fmt"
"github.com/leonelquinteros/gotext"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could import this as i18n, so that instead of calling gotext.Get() we would call i18n.Get().
This may be clearer when browsing the code?

@marckhouzam
Copy link
Collaborator

marckhouzam commented Dec 23, 2023

Something to discuss later, I just don’t want to forget.

We will need a way for a program using cobra to disable these translations. For example, if a mycli project does not translate its own strings, it might not want the cobra strings to get translated as it would give an inconsistent user experience.

So we probably want the project to explicitly have to turn on translations through a global setting.

@Goutte
Copy link
Author

Goutte commented Dec 23, 2023

Thanks for the comprehensive review, @marckhouzam !

Also, I'm also interested in understanding how a project using Cobra would proceed to add translations, or is that completely up to the project and this PR does not help?

This PR does not help devs to implement their I18N.
They are free to choose whatever solution they like.

We could perhaps provide some doc/sugar on how to use the same translation toolkit as cobra, since the relevant libraries are already included ?

Can you clarify what you mean by "Make sure your software has also updated the MO files"? Do we absolutely need to use a special editor like poedit or can this be done from the command-line?

Good question. Since gettext is mature, I'm pretty confident there are CLI tools out there handling this. (or we'll have to make one, preferably in Golang, using cobra of course)

I'd like the MO to be generated by CI as well, if only to prevent contributors to inject nasty things in the binary files.

I'll look into this.

So we probably want the project to explicitly have turn on translations through a global setting.

Very good point. Partially translated apps are awkward, which is precisely why I wanted cobra to support i18n. I'm not sure right now how to proceed to add a global setting to cobra, but I'll look into it and come back to ask for help if I don't find how. Do give tips if you'd prefer it to be done in some specific way :)

@marckhouzam
Copy link
Collaborator

To allow a project to enable translations I’m thinking of two approaches you can take:

  1. A global setting like cobra has for other features:

    cobra/cobra.go

    Lines 52 to 66 in 4122785

    // EnablePrefixMatching allows setting automatic prefix matching. Automatic prefix matching can be a dangerous thing
    // to automatically enable in CLI tools.
    // Set this to true to enable it.
    var EnablePrefixMatching = defaultPrefixMatching
    // EnableCommandSorting controls sorting of the slice of commands, which is turned on by default.
    // To disable sorting, set it to false.
    var EnableCommandSorting = defaultCommandSorting
    // EnableCaseInsensitive allows case-insensitive commands names. (case sensitive by default)
    var EnableCaseInsensitive = defaultCaseInsensitive
    // EnableTraverseRunHooks executes persistent pre-run and post-run hooks from all parents.
    // By default this is disabled, which means only the first run hook to be found is executed.
    var EnableTraverseRunHooks = defaultTraverseRunHooks
  2. A field in the root command like we have for tuning shell completions
    CompletionOptions CompletionOptions

if you think different settings might be useful eventually, I would recommend using a struct so we could grow it overtime

@marckhouzam
Copy link
Collaborator

This PR does not help devs to implement their I18N. They are free to choose whatever solution they like.

We could perhaps provide some doc/sugar on how to use the same translation toolkit as cobra, since the relevant libraries are already included ?

Thanks for clarifying, it makes sense. Let’s leave this for a potential separate PR and focus on cobra’s strings .

@Luap99
Copy link
Contributor

Luap99 commented Dec 28, 2023

I think cobra should have a built tag to disable/enable translations. Right now it looks like all translations are embedded in the final binary. That will lead to unnecessary bloat for users who do not want to use it so a build tag would be the better option IMO.

And these translations should be opt in not opt out IMO.

@marckhouzam
Copy link
Collaborator

I think cobra should have a built tag to disable/enable translations IMO.

Good idea +1

And these translations should be opt in not opt out IMO.

Agreed 👍

Copy link

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.

Copy link

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.

@Goutte
Copy link
Author

Goutte commented Jan 10, 2024

I think cobra should have a built tag to disable/enable translations. Right now it looks like all translations are embedded in the final binary. That will lead to unnecessary bloat for users who do not want to use it so a build tag would be the better option IMO.

And these translations should be opt in not opt out IMO.

That's a good idea.

I think I managed to do this using the "locales" build tag.

Now I need to tweak the test suite to use that build tag,
or perhaps even make two runs, with and without.

Anyhow, for now the tests are broken until I figure this out.

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cobra-command Core `cobra.Command` implementations kind/feature A feature request for cobra; new or enhanced behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants