search

urfave / cli

A simple, fast, and fun package for building command line apps in Go
https://cli.urfave.org
MIT License
21.91k stars 1.69k forks source link

Feat: Add minimal support for i18n #1789

Closed dearchap closed 1 year ago

dearchap commented 1 year ago

What type of PR is this?

(REQUIRED)

What this PR does / why we need it:

(REQUIRED)

Enable basic internalization support for urfave/cli

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #980

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)


This adds basic framework support for internationalization for urfave/cli. Right now the languages built in are en-US/en-GB which zero changes between the messages. As people request more languages the capability to add this is present with no changes to core code base
meatballhat commented 1 year ago

Ack sorry about the conflicts! 😭

github-actions[bot] commented 1 year ago

[This comment was generated by JFrog Frogbot 🐸](https://github.com/jfrog/frogbot#readme)
dearchap commented 1 year ago

@meatballhat

meatballhat commented 1 year ago

@dearchap Thank you! I sincerely appreciate your work on this 🤘🏼

I'm still confused about the intended usage given this line in the generated code. How is this supposed to work if urfave/cli/v3 is being used by an application that also uses code generated by gotext?

Given that the template file for the generated code hasn't been touched in ~4 years, I'm wondering if using golang.org/x/text/message/pipeline (via gotext) is the right move 🤔

dearchap commented 1 year ago

@meatballhat you have a keen eye. I tested cli with these changes along with a sample app that uses its own catalog and basically the translation doesn't work as one catalog overrides the other due to the message.DefaultCatalog in init. I'm going to redo this PR.

dearchap commented 1 year ago

@meatballhat I've hacked around the package global Catalog initialization. I dont like it but this is what we have until support for multiple Catalogs is rolled into the text/message package itself. https://cs.opensource.google/go/x/text/+/refs/tags/v0.10.0:message/catalog/catalog.go;l=281

For now this is the simplest route without adding dependencies on other i18n libraries which look to me more heavyweight

dearchap commented 1 year ago

@meatballhat Also I'm not sure why only the window tests are failing. Can you take a look ?

meatballhat commented 1 year ago

@meatballhat Also I'm not sure why only the window tests are failing. Can you take a look ?

Offhand I don't see why that failure is happening 🤔 Can the make go-generate step be limited to most recent Go on ubuntu like with some of the nearby steps?