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

Add internationalization function #1522

Closed
wants to merge 0 commits into from
Closed

Add internationalization function #1522

wants to merge 0 commits into from

Conversation

bin-ly
Copy link

@bin-ly bin-ly commented Mar 19, 2024

Use the fluent library to add internationalization functions to the fd project: the translations directory displays the resources required for translation. The i18.rs file is a file that implements internationalization. It obtains the system language through the get_locale() method and displays text according to the language. For example, if the system language is en-US, Chinese will be displayed. If there is no language translation resource (translations directory) set by the system, English will be displayed by default.

Copy link
Collaborator

@tmccombs tmccombs left a comment

Choose a reason for hiding this comment

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

I'm concerned about the impact this will have on performance, especially startup performance, since it will require opening, reading, and parsing a fluent translations file.

Is there any precedent for doing something like this in a simple cli program.

This PR primarily deals with the help text. I wonder if we could get most of the benefit from this, with less impact on performance, just by translating the man page, and maybe using translated help texts if the locale is non-english and the --help or -h option is used.

Another concern I have is maintenance. How will we keep these translations up to date? Especially since, AFAIK, none of the maintainers understands chinese?

Cargo.toml Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

What impact will this have on the size of the executable, and the performance of the application.

Copy link
Author

Choose a reason for hiding this comment

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

1.Executable file size: The executable file size is 72M before internationalization is added, and 79M after internationalization is added. The strings that fd needs to be internationalized do not seem to be particularly large and will not cause the executable file to be too large.
2.Regarding performance: I used the hyperfine benchmark tool for comparison, and the performance difference is not particularly big.
result
3.Regarding maintenance: I am from China and am very interested in rust and fd projects. If the project requires maintainers who understand Chinese, I will be very happy to participate.

src/cli.rs Outdated
@@ -22,10 +23,12 @@ use crate::filter::SizeFilter;
#[command(
name = "fd",
version,
about = "A program to find entries in your filesystem",
after_long_help = "Bugs can be reported on GitHub: https://github.com/sharkdp/fd/issues",
about = translate("A_program_to_find_entries_in_your_filesystem"),
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 probably use more succinct keys for the translations.

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 okay. I was thinking that developers would be more intuitive when reading the code, but it seems that they can also follow the prompts to look at the translations directory.

src/i18.rs Outdated
use sys_locale::get_locale;
use unic_langid::LanguageIdentifier;

pub fn translate(change: &str) -> String{
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this called "change" would "message" be a better name?

Copy link
Author

Choose a reason for hiding this comment

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

it seems "message" is better

src/i18.rs Outdated
let langid: LanguageIdentifier = locale.parse().expect("wrong language");
let locales = vec![langid.into()];
let resources = vec!["message.ftl".into()];
let mgr = ResourceManager::new("./translations/{locale}/{res_id}".into());
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this work? what is this path relative to? Will it work when fd is installed as a system package? if the working directory is an arbitrary directory?

From a quick look at the source of ResourceManager it looks like this will just be from the current working directory, which in most cases won't be what we want. I think this is at least one reason why most of the tests are failing.

It will need to somehow reference the data directory, which is going to depend on how fd was installed, and what operating system it was.

In many cases it needs to be relative to the location of the fd executable. But if was installed as a system package, it will need to reference /usr/share/fd.

I'm not sure what the best way to handle that is. It would probably be worth looking at what other CLI apps do.

Are there any other CLL apps that use fluent like this?

Copy link
Author

Choose a reason for hiding this comment

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

This part is to obtain international resources, that is, the translation files required for internationalization. The current processing method is to store the translations directory in the /usr/share/fd directory, and modify the code here to the absolute path, that is, "/usr/share/fd/translations/{locale}/{res_id}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current processing method is to store the translations directory in the /usr/share/fd directory, and modify the code here to the absolute path, that is, "/usr/share/fd/translations/{locale}/{res_id}"

It would be better if the packaging process doesn't require modifying the code.

And what if it is just installed from a zip file at an unknown location. How will fd know where the translation files are?

What happens if it can't find the translation files?

src/i18.rs Outdated
let locales = vec![langid.into()];
let resources = vec!["message.ftl".into()];
let mgr = ResourceManager::new("./translations/{locale}/{res_id}".into());
let bundle = mgr.get_bundle(locales, resources);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having to create a new bundle every time we call translate, we should only create the bundle once.

We should use a OnceCell to lazily initialize the a ResoruceBundle the first time it is needed, and then re-use that instance afterwards.

Copy link
Author

Choose a reason for hiding this comment

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

I may need to research this area again. It seemed like setting a global variable so that it could only be called once did not work.

Copy link
Collaborator

@tmccombs tmccombs Apr 1, 2024

Choose a reason for hiding this comment

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

I think you would have something like

static resource_manager:  OnceCell<ResourceManager> = OnceCell::new();
static bundle: OnceCell<FluentBundle<&'static FluentResource> = OnceCell::new();


// and then in this function you would have something like:

let mgr = resource_manager.get_or_init(|| {
   // we still need to fix this path...
   ResourceManager::new("./translations/{locale}/{res_id}".into()); 
});

let bundle = bundle.get_or_init(|| {
  let locale = get_locale().unwrap_or_else(|| String::from("en-US"));
   let langid: LanguageIdentifier = locale.parse().expect("wrong language"); 
    let locales = vec![langid.into()];
    let resources = vec!["message.ftl".into()];
   mgr.get_bundle(locales, resources)
});

src/i18.rs Outdated
let pattern = value.value().expect("Message has no value.");

let mut errors = vec![];
let msg = bundle.format_pattern(&pattern, None, &mut errors).to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid the to_string if possible. If the bundle is moved into a static OnceCell then we can return the Cow<str> that format_pattern returns.


Bugs_can_be_reported_on_GitHub = Bugs can be reported on GitHub: https://github.com/sharkdp/fd/issues

Print_helps = Print helps
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Print_helps = Print helps
Print_help = Print help

Copy link
Author

Choose a reason for hiding this comment

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

Fluent does not support spaces between translated strings, so ‘_’ is used instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My point is it should be "help" not "helps"

Copy link
Collaborator

Choose a reason for hiding this comment

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

the Makefile will need to be updated to include these files in an installation.

@bin-ly bin-ly requested a review from tmccombs March 21, 2024 07:31
@bin-ly bin-ly closed this Apr 15, 2024
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.

2 participants