gotext icon indicating copy to clipboard operation
gotext copied to clipboard

Add support for multiple languages

Open m-horky opened this issue 1 year ago • 6 comments

Is this a fix, improvement or something else?

This is an improvement, an updated version of #73.

This patch adds support for fallback translations. pt_BR:pt:es makes it possible to use Brazilian Portuguese, fallback to default Portuguese and to Spanish when the translation is not available.

What does this change implement/fix?

  • This patch changes the config struct, and internal implementation of functions that work with it.
  • It changes Get*() functions to iterate over loaded languages, instead of using the only one.
  • It changes IsTranslated*() functions to include new required argument of the language to be checked.
  • See more detailed changes in the respective commits.

The original PR failed because PR #80 was merged first. Since my PR changes the naming from storage to locales, I'd like to have opinion of @didrocks as well. For this version I have opted to keep their function names GetStorage and SetStorage for complete backwards compatibility. This is a bit unfortunate because the functions are called differently from the rest of the code, but we cannot change that without making a breaking change. (I am not blaming you, the change does make sense. The naming just is not the best, as it is a bit cryptic without knowing the internals.) I can revert it all to 'storage' if consistency is preferred, that's up to @leonelquinteros to tell :)

I have ...

  • [x] answered the 2 questions above,
  • [x] discussed this change in an issue,
  • [x] included tests to cover this changes.

m-horky avatar Aug 30 '23 08:08 m-horky

The original PR failed because PR https://github.com/leonelquinteros/gotext/pull/80 was merged first. Since my PR changes the naming from storage to locales, I'd like to have opinion of @didrocks as well. For this version I have opted to keep their function names GetStorage and SetStorage for complete backwards compatibility. This is a bit unfortunate because the functions are called differently from the rest of the code, but we cannot change that without making a breaking change.

Oh, I completely got it that this was the system locales (I had to dig quite in the code for the next couple of PR I submitted if you saw that :p). I was puzzled by the naming too and I agree that, from an external contributor POV, a slice of locales makes more sense, but as you told, that’s up to @leonelquinteros.

On the renaming, as there has been no stable release with it, I have no issue changing my projects using those commits if the Get/Set names were changed here.

didrocks avatar Aug 30 '23 08:08 didrocks

@leonelquinteros ping :) Since no version has been released since Dec 22, what do you think about the storage → locale rename? Either way my PR is ready for review notes. I'd appreciate if you found some time for that. I think the package would benefit from having multi-language capability.

m-horky avatar Oct 09 '23 12:10 m-horky

I really want this for cobra ( spf13/cobra#2090 ), since I want to fall back to english when a translation is not found, as well as support regional fallback (fr_CA → fr).

I reviewed this, and besides a corner-case happening when SetLocales is kind of abused by the consumer, it looks good to me. :)

Goutte avatar Dec 16 '23 09:12 Goutte

Thanks for catching that @Goutte. Fixed.

m-horky avatar Dec 19 '23 09:12 m-horky

Thank you, @m-horky ! I'm eager to try this out :partying_face:

Goutte avatar Dec 19 '23 12:12 Goutte

Hey guys, I'm very sorry for the delay on this. I'm glad you've advanced with this PR so far and cross-reviewed it. I'm reviewing it now and will merge soon.

About renaming... As long as the public interface isn't changed, I'm OK with renaming internals as long as they make sense and don't add even more confusion to the matter. The thing about the public API is that to follow Golang guidelines, if we change anything there we need to move to a v2 structure which I'm trying to avoid because of maintainability cost. That said, I'm OK to add new methods that "replace" or "replicate" functionality from others that may have confusing names, but we still need to support the old API.

leonelquinteros avatar Dec 21 '23 12:12 leonelquinteros