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

Feat: Add minimal support for i18n #1789

Closed
wants to merge 7 commits into from
Closed

Conversation

dearchap
Copy link
Contributor

What type of PR is this?

(REQUIRED)

  • feature

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

@dearchap dearchap requested a review from a team as a code owner June 28, 2023 20:27
@meatballhat
Copy link
Member

Ack sorry about the conflicts! 😭

@github-actions
Copy link


  • Frogbot also supports Contextual Analysis, Infrastructure as Code Scanning and Secrets Detection. These features are included as part of the JFrog Advanced Security package, which isn't enabled on your system.

Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

  • This is very exciting! 🎉
  • It looks like the generated catalog code sets the message.DefaultCatalog package-level variable 🤔 Does this mean that this library is intended for use at the application level and not from a dependency like github.com/urfave/cli/v3?
  • Is there supposed to be an import of _ "github.com/urfave/cli/v3/internal/translations" somewhere in the top level so that the catalog is initialized?
  • I had to manually install gotext to get this working locally. Can a gotext-ensuring command be added to internal/build/build.go like the one that ensures goimports exists?

@dearchap
Copy link
Contributor Author

@meatballhat

  • Addressed all the conflicts
  • Added import of _ "github.com/urfave/cli/v3/internal/translations"
  • Added ensure-gotext to internal build
  • Added ensure-gotext and go-generation to the build system

@meatballhat
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@meatballhat
Copy link
Member

@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?

@dearchap dearchap closed this Jul 4, 2023
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.

i18n support for urfave/cli
2 participants