Add i18n by using rust i18n #807

Mastermindaxe · 2024-05-19T07:48:43Z

Standards checklist:

Fixes #2

src/main.rs

SteveLauC · 2024-05-25T09:02:05Z

Hi, I hope the review is not too late, thanks again for working on it! :)

But tbh I'm not quite satisfied with the usability of that crate in println macros for example. It's very tedious as println!("Hello") becomes println!("{}", t!("Hello")). This makes it much more unreadable imo. Especially in print statements where there are many arguments in the format string.

Yeah, this is indeed a problem, but I think it is ok to use it in our first implementation, in the future, we can probably provide a custom macro to make it easier to use.

I think it works well enough though. If you could take a quick glance and tell me if you want me to move forward with it, that would be amazing. I'm just afraid that there might be a better option that sacrifices developer experience less. I'm open to suggestions and any input you have. If that's a non-issue for you I'd be happy to finish the PR and migrate the still missing strings.

I think this PR is pretty good, we are on the right track!

One further note on the implementation: I've use the english string literals as the translation keys for better developer experience.

Yeah, using English as the key and the fallback option makes sense to me.

SteveLauC · 2024-05-25T09:02:50Z

BTW, you can rebase your branch to fix the test coverage CI issue.

Mastermindaxe · 2024-05-26T21:00:26Z

Hi, I hope the review is not too late, thanks again for working on it! :)

No worries! We all have a life :D

Yeah, this is indeed a problem, but I think it is ok to use it in our first implementation, in the future, we can probably provide a custom macro to make it easier to use.

Sounds like a plan! I'd be happy to design sth with you for good dx

I think this PR is pretty good, we are on the right track!

I'll work more on it this/next week (depending on your timezone)

Yeah, using English as the key and the fallback option makes sense to me.

👍 Seemed the most appropriate for DX

Mastermindaxe · 2024-05-31T08:52:14Z

@SteveLauC Still working on this. One thing I already mentioned but which I find troubling still, is that for complex statements regarding debug formatting the code gets quite ugly:
Before:

debug!("Detected {:?} as {:?}", &path, &binary_name);

After:

debug!("{} {:?} {} {:?}", t!("Detected"), &path, t!("as"), &binary_name);

I haven't found a good way to deal with this as the crate doesn't allow to pass in specific formatting hints for a variable. Is there a way to call object.debug_display() or sth to display the debug formatting that I'm not aware of to make this easier?
Ie:

debug!("{}", t!("Detected {:?} as {:?}", path=&path.debug_fmt(), binary_name=&binary_name.debug_fmt());

Hint: The {:?} is just part of the key to make it easier to translate against but has no meaning in the terms of the macro invocation

SteveLauC · 2024-06-01T10:55:36Z

debug!("{}", t!("Detected {:?} as {:?}", path=&path.debug_fmt(), binary_name=&binary_name.debug_fmt());

To use parameters, looks like we have to define the whole sentence as a localized text:

_version: 2

"binary-detection":
  en: "Detected %{path} as %{binary}"

then in Rust code, we would:

debug!("{}", t!("binary-detection", path = format!("{:?}", &path), binary = format!("{:?}", &binary_name))

Honestly, this would make the code harder to read since we don't know what the actual string would look like unless we take a look at the localized file.

And for texts like "Detected {} as {}", I think we should use the whole sentence as a localized text rather than defining texts for every word since blindly translating those words probably won't make a natural sentence. For example, an ideal translation for "Detected ~/.cargo/bin/cargo as cargo" in simplified Chinese (zh-cn) would be:

"检测到 cargo: ~/.cargo/bin/cargo"

Which literally means "I have found cargo: it is located in ~/.cargo/bin/cargo" in English. However, blindly translating those word looks like this:

"检测到 ~/.cargo/bin/cargo 作为 cargo"

which feels quite weird...

Given the above problem, I think we should stop using words as localized texts and take whole sentences as the smallest units, your thoughts on this?

For the readability issue, I don't have a good solution for this considering that the t!() macro has to take localized text key, maybe we can add comments for those code? i.e., something like:

// Detected {path} as {binary}
debug!("{}", t!("binary-detection", path = format!("{:?}", &path), binary = format!("{:?}", &binary_name))

src/breaking_changes.rs

src/main.rs

src/self_update.rs

src/config.rs

src/steps/remote/vagrant.rs

src/terminal.rs

src/error.rs

Mastermindaxe · 2024-06-09T20:56:32Z

@SteveLauC I'm still working on this. I'm quite unsatisfied with the DX so far 🤔 I'll think about your questions and will get back to you with another suggestion on how we can do things properly and without much friction for contributors 👍

Mastermindaxe · 2024-06-12T08:33:28Z

Given the above problem, I think we should stop using words as localized texts and take whole sentences as the smallest units, your thoughts on this?

Absolutely agree!

For the readability issue, I don't have a good solution for this considering that the t!() macro has to take localized text key, maybe we can add comments for those code? i.e., something like:
// Detected {path} as {binary}
debug!("{}", t!("binary-detection", path = format!("{:?}", &path), binary = format!("{:?}", &binary_name))

I'd suggest we use the following for the example:

debug!("{}", t!("Detected {path} as {binary}", path = format!("{:?}", &path), binary = format!("{:?}", &binary_name))

Reasoning behind this being that I think comments are bound to become obsolete and outdated bc there is no check if they are actually accurate and up-to-date. What about using the whole string as we have before (bc of readability and dx) and just put down the placeholder names. This does not guarantee that the placeholder won't eventually become outdated. But what it does help with is readability. It also eliminates the need for going back and forth to the translation dictionary bc it's right in the translation key. This also keeps in line with our approach for simple keys.

That said, I'm still not sold on the readability issue regarding the format! macro for displaying debug messages, but it is what it is 😅

I'll start shifting my initial implementation to this if you don't have any concerns.

Minor thing: We could also think about using {path:?} to indicate that we are using a debug representation, but I think that might be confusing and a little convoluted. Best to stick to simple syntax agreeements imo.

Only thing that might be confusing is for new contributors who think that our pseudo placeholder is actually a placeholder with meaning behind it. Which it isn't 😄 Maybe we could add a part to CONTRIBUTING.md that explains a bit for adding translations? 🤔 What do you think?

Sorry for the wall of text! Hope you have a great week and just let me know what you think of these changes. I'll try to find the time to get this thing to a close

SteveLauC · 2024-06-15T13:40:03Z

I'd suggest we use the following for the example:

debug!("{}", t!("Detected {path} as {binary}", path = format!("{:?}", &path), binary = format!("{:?}", &binary_name))

Yeah, this is much better in readability!

That said, I'm still not sold on the readability issue regarding the format! macro for displaying debug messages, but it is what it is 😅

I just tried to write a custom debug! macro that converts:

custom_debug!("Detected {:?} as {:?}", &path, &binary_name)

into

debug!("{}", t!("Detected {path} as {binary_name}", path = format!("{:?}", &path), binary_name = format!("{:?}", &binary_name)))

But I am not am expert in macros and ChatGPT says that this is not easy to implement since it needs to involve strings operation, but Rust macro operates in tokens.

Taking a look at the changed code, seems like all we need is a custom format! macro, then we can implement other common macros like println! debug! using it.

Minor thing: We could also think about using {path:?} to indicate that we are using a debug representation, but I think that might be confusing and a little convoluted. Best to stick to simple syntax agreeements imo.

This looks not bad IMO, since this is a typical pattern that is commonly supported by Rust std macros.

Only thing that might be confusing is for new contributors who think that our pseudo placeholder is actually a placeholder with meaning behind it. Which it isn't 😄 Maybe we could add a part to CONTRIBUTING.md that explains a bit for adding translations? 🤔 What do you think?

Yeah, we should document the approach we take in CONTRIBUTING.md no matter what it is.

Sorry for the wall of text! Hope you have a great week and just let me know what you think of these changes. I'll try to find the time to get this thing to a close

No need to be sorry at all! :)

Mastermindaxe · 2024-06-24T14:27:24Z

Updated the existing commit. Still a few todos left, but I think that should be the bulk of it 👍 Would love some assistance on double-checking the translation placeholders etc. Slowly but surely my eyes are getting worn out looking at yaml 😄

Now on to the todo items and rebasing from main!

Mastermindaxe · 2024-06-24T14:34:26Z

Open Todo-items:

Go over the added todo items in the code and evaluate or implement them (will do that in the next few days)
Rebase onto main
Edit CONTRIBUTING.md
Edit the default PR template to include a reminder to translate new steps (@SteveLauC Would be happy to have your input on this. I think it would be a nice addition)
Define next steps for testing that i18n works and when to merge the changes to main and when to release after that. This is to make sure that I've not broken some strings when adding in i18n. My head hurts, so that very well could be a possibility 😄
Implement the starting point of i18n. Ie how to get the system locale
Cleanup and sort the app.yml to remove duplicated or unneeded keys
Any changes needed for the CHANGELOG? As in squashing commits or editing commit messages? Couldn't find a list or tutorial in CONTRIBUTING.md

@SteveLauC If I'm missing some steps here feel free to add some 👍 I'll add these todo items to the main comment so it shows up on the overview of this PR once you give me a thumbs-up for it

Mastermindaxe · 2024-06-24T14:36:17Z

I'll go over the diff later this week to try and catch easy mistakes I made. I don't mind if you do a proofread in the meantime, just be aware that there might be hasty mistakes not yet fixed

SteveLauC · 2024-06-24T23:47:56Z

Edit the default PR template to include a reminder to translate new steps (@SteveLauC Would be happy to have your input on this. I think it would be a nice addition)

Hi, thanks for reminding me of this!

I think something like the following would be good:

This PR updates locales/app.yml if if modifies the strings that are used to interact with users

Any changes needed for the CHANGELOG?

Topgrade does not have a changelog file, though we record breaking changes in BREAKINGCHANGES_DEV.md, this PR should be considered a breaking change for those non-English users, they may use scripts to involve Topgrade and the script may depend on Topgrade's output.

As in squashing commits or editing commit messages?

We use squash merge, so it will always be 1 commit in the main branch, no need to worry about these things:)

Couldn't find a list or tutorial in CONTRIBUTING.md

Maybe I should document the merge style in CONTRIBUTING.md.

SteveLauC · 2024-06-25T00:29:38Z

#[should_panic(expected = "Version numbers can not be all 0s")] // TODO: Problems with i18n?

Looks like this is something impossible? expected needs a string literal, and i18n is a runtime ting, so I think we can remove it.

// TODO: i18n of clap currently not easily possible. Waiting for clap-rs/clap#380

Yeah, we can leave a TODO here.

#[error("A step failed")] // TODO: How to add i18 to these?

Looks like we need to manually implement the std::error::Error trait for the error types:

#[derive(Debug)]
pub struct StepFailed;

impl std::fmt::Display for StepFailed {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "A step failed") 
    }
}

impl std::error::Error for StepFailed { }

print!("{}", config::EXAMPLE_CONFIG); // TODO: Find a way to use a translated example config

Looks like we need a toml parser that can be used to replace comments?

// TODO: Refactor this to make it easier to implement i18n
// Ie use the first letter from the translations, not a hardcoded literal
print_info("\n(R)eboot\n(S)hell\n(Q)uit");

use the first letter from the translations

Using the first letter may only work for some languages, I think we need to have a custom transaction for every language. For English, the original string is good enough. For Chinese, I think it should probably be something like:

\n(R)重启\n(S)Shell\n(Q)退出

As you can see, for reboot, I am still using the R as the confirm char, and Shell is not translated, so the confirm char is still S, and there are 2 Ss in the prompt string for the consistency with other R(重启) and Q(退出).

For the confirm keys, we may need to define some variables for them as well:

"\n(R)eboot\n(S)hell\n(Q)uit":
  en: "\n(R)eboot\n(S)hell\n(Q)uit"
  zh-CN: "\n(R)重启\n(S)Shell\n(Q)退出"

"confirm_char_for_reboot_uppercase":
  en: "R"
  zh-CN: "R"
"confirm_char_for_reboot_lowercase":
  en: "r"
  zh-CN: "r"

Topgrade also has a prompt for Y(es)/N(o), it should be done in the similar way:)

// TODO: Implement i18n for this using the first character from the translated key

println!("{body}"); // TODO: Any way to translate what I think are the release notes?

Emmm, this is indeed something hard...

// TODO: How to handle this with i18n in mind?
let re = Regex::new(r"* '(.?)' for '(.?)' is outdated").unwrap();

This is something I am not willing to translate, if the underlying step supports locale, and Topgrade needs to capture its output, and does parsing on it, we should let the output in English or we will have to parse for every language, which is quite tedious, also see: https://github.com/topgrade-rs/topgrade/blob/main/CONTRIBUTING.md#some-tips

// Skip causes
// TODO: Put them in a better place when we have more of them
// TODO: Translate this
pub const REQUIRE_SUDO: &str = "Require sudo or counterpart but not found, skip";

We can use Lazy to initialize a static string at runtime.

pub static REQUIRE_SUDO: Lazy<String> = Lazy::new(|| {
  t!("Require sudo or counterpart but not found, skip")
})

Mastermindaxe · 2024-06-27T17:59:31Z

Looks like this is something impossible? expected needs a string literal, and i18n is a runtime ting, so I think we can remove it.

In theory we could implement the thing ourselves just like we did with the error Impls. I don't think that's a very good idea though. I'm content with not translating that for the moment. I'm sure the benefit of a 99% translated software (or worst case 90%) is still very much palpable than not having any translation at all.

I just now saw that this code block is in a testing module anyway. Absolutely no need to translate this imo as the agreed-upon language in software development is English anyway. No problem here 😄

Looks like we need a toml parser that can be used to replace comments?

I'd say we just have multiple toml files. Seems way easier imo 🤔 You can check my most recent push. I've added the config file translation(s)

Using the first letter may only work for some languages, I think we need to have a custom transaction for every language. For English, the original string is good enough. For Chinese, I think it should probably be something like:

We could either do that or just not translate those keys at all 🤔 It shouldn't keep people from using the software imo as it would be clear on what key to press. But we could also do that with the custom key thing. I don't mind either. Just thinking in terms of maintainability

println!("{body}"); // TODO: Any way to translate what I think are the release notes?

Imo not sth we could feasibly accomplish. We would have to translate commits and the release notes for every release. I don't think that's quite possible or that the benefit of doing that warrants the costs of maintaining that. You'd need maintainer or trusted people to check that as you (probably 😄) don't speak all the translated languages.

This is something I am not willing to translate, if the underlying step supports locale, and Topgrade needs to capture its output, and does parsing on it, we should let the output in English or we will have to parse for every language, which is quite tedious, also see: https://github.com/topgrade-rs/topgrade/blob/main/CONTRIBUTING.md#some-tips

Absolutely agree! 👌

We can use Lazy to initialize a static string at runtime.

We could also not make that a static string 🤔 Is there too much overhead in doing that? I don't think it would be. I think the cost of adding sth like the lazy macro is higher than just plopping a t!("thingamajig") in all the places that it's used and doing that lookup at runtime. What do you think?

Mastermindaxe · 2024-06-27T18:03:11Z

Another note: I'd like to add issues to open todo-items in hopes of better tracking them. Ie. the following code:

// TODO: i18n of clap currently not easily possible. Waiting for https://github.com/clap-rs/clap/issues/380

would get a link to an issue in this repository for i18n for the command line interface

Mastermindaxe · 2024-06-27T18:09:49Z

And one last thing regarding /src/steps/git.rs:
I don't think there is a good way to deal with the colors and internationalization tbh 🤔 As the resulting objects are not Strings I don't think we can use that with i18n. I see two options:

Single word translations. Which we already agreed upon we don't want to use as it introduces non-solvable problems for certain languages.
We just color the whole sentence. Ie translate first and then color the whole thing. That would be my preferred way of doing ti. Question is if that is sth you want to do considering that there had to be a reason for color-coding the text in this specific way in the first place.

Let me know what you think!

SteveLauC · 2024-06-30T09:16:55Z

Looks like this is something impossible? expected needs a string literal, and i18n is a runtime ting, so I think we can remove it.

I just now saw that this code block is in a testing module anyway. Absolutely no need to translate this imo as the agreed-upon language in software development is English anyway. No problem here 😄

Yeah, I agree.

Looks like we need a toml parser that can be used to replace comments?

I'd say we just have multiple toml files. Seems way easier imo 🤔 You can check my most recent push. I've added the config file translation(s)

Emmm, putting the whole configuration file as a translation unit does not seem good. When we have multi-language support, this file will be duplicated many time, and there is no fallback for specific entry, you need to either translate all the entries or don't do it at all. Though I don't have a good approach, I think we can leave it not translated if there is no good way to do it.

Using the first letter may only work for some languages, I think we need to have a custom transaction for every language. For English, the original string is good enough. For Chinese, I think it should probably be something like:

We could either do that or just not translate those keys at all 🤔 It shouldn't keep people from using the software imo as it would be clear on what key to press. But we could also do that with the custom key thing. I don't mind either. Just thinking in terms of maintainability

Yeah, Having those confirm keys not translated looks good to me, and it can make maintenance easier.

println!("{body}"); // TODO: Any way to translate what I think are the release notes?

Imo not sth we could feasibly accomplish. We would have to translate commits and the release notes for every release. I don't think that's quite possible or that the benefit of doing that warrants the costs of maintaining that. You'd need maintainer or trusted people to check that as you (probably 😄) don't speak all the translated languages.

Yeah, let's ignore it.

This is something I am not willing to translate, if the underlying step supports locale, and Topgrade needs to capture its output, and does parsing on it, we should let the output in English or we will have to parse for every language, which is quite tedious, also see: https://github.com/topgrade-rs/topgrade/blob/main/CONTRIBUTING.md#some-tips

Absolutely agree! 👌

:)

We can use Lazy to initialize a static string at runtime.

We could also not make that a static string 🤔 Is there too much overhead in doing that? I don't think it would be. I think the cost of adding sth like the lazy macro is higher than just plopping a t!("thingamajig") in all the places that it's used and doing that lookup at runtime. What do you think?

It looks good to me to directly use t!(), Topgrade is not something desperate for performance.

Another note: I'd like to add issues to open todo-items in hopes of better tracking them. Ie. the following code:

Yeah, we can have a tracking issue for this.

And one last thing regarding /src/steps/git.rs: I don't think there is a good way to deal with the colors and internationalization tbh 🤔 As the resulting objects are not Strings I don't think we can use that with i18n. I see two options:

Single word translations. Which we already agreed upon we don't want to use as it introduces non-solvable problems for certain languages.

We just color the whole sentence. Ie translate first and then color the whole thing. That would be my preferred way of doing ti. Question is if that is sth you want to do considering that there had to be a reason for color-coding the text in this specific way in the first place.

Let me know what you think!

Do you mean things like this?

- println!("{} {}", style("Up-to-date").green().bold(), repo.as_ref().display());
+ // TODO: Properly use i18n in this one. How to deal with the colors?
+ println!("{} {}", style(t!("Up-to-date")).green().bold(), repo.as_ref().display());

I don't think there is a good way to deal with the colors and internationalization tbh

What is the problem with the strategy presented in this PR? i.e., the above diff :D

Mastermindaxe · 2024-07-01T12:11:05Z

Emmm, putting the whole configuration file as a translation unit does not seem good. When we have multi-language support, this file will be duplicated many time, and there is no fallback for specific entry, you need to either translate all the entries or don't do it at all. Though I don't have a good approach, I think we can leave it not translated if there is no good way to do it.

You are right I think. The maintenance overhead alone is too big when there is a new config parameter the config has to be updated by the language maintainers before a user gets a new and correct version of the config 🤔
We could construct a single massive string and inject the comments that way. Ie:

...
t!("config.include_comment") +
"[include]"
...

I think that should work easily enough, but it's a bit tedious to implement :D

Yeah, Having those confirm keys not translated looks good to me, and it can make maintenance easier.

Alright! Will implement this then 👍

EDIT: Done ✅

Do you mean things like this?

Sth more like this:

// TODO: Properly use i18n in this one. How to deal with the colors?
          println!(
              "\n{} {}\n",
              style(t!("Only")).green().bold(),
              t!("updated repositories will be shown...")
          );

Here we are back to translating singular words and not the whole sentence. I don't think there's a clever way to only highlight a portion of the translated text 🤔
(You picked the one example where it works flawlessly 😄)
I'd suggest just highlighting the whole sentence but this has usability implications beyond i18n. What do you suggest/prefer?

Mastermindaxe · 2024-07-04T09:37:45Z

Example for the config translation stuff:

use rust_i18n::t;

pub fn construct_default_config() -> String {
    t!("include").to_string()
        + r#"
    [include]
    # paths = ["/etc/topgrade.toml"]


    [misc]
    "#
    + t!("pre_sudo").as_ref()
    + "# pre_sudo = false"
    + t!("sudo_command").as_ref()
    + r#"# sudo_command = "sudo""#
}

and the locales/config.yaml:

"include":
  en: |
    # Include any additional configuration file(s)
    # [include] sections are processed in the order you write them
    # Files in $CONFIG_DIR/topgrade.d/ are automatically included before this file
"pre_sudo":
  en: |
    # Run `sudo -v` to cache credentials at the start of the run
    # This avoids a blocking password prompt in the middle of an unattended run
    # (default: false)
"sudo_command":
  en: "# Sudo command to be used"

@SteveLauC What do you think of this? Should I continue with this or do you see a different approach? 🤔

locales/app.yml

SteveLauC · 2024-07-06T04:54:29Z

You are right I think. The maintenance overhead alone is too big when there is a new config parameter the config has to be updated by the language maintainers before a user gets a new and correct version of the config 🤔
We could construct a single massive string and inject the comments that way. Ie:
...
I think that should work easily enough, but it's a bit tedious to implement :D

Example for the config translation stuff:
...
and the locales/config.yaml:
...
@SteveLauC What do you think of this? Should I continue with this or do you see a different approach? 🤔

This is better than translating the whole file, but it is still kinda, I wonder can we skip it for now and leave a TODO in the tracking issue?

Sth more like this:
...
Here we are back to translating singular words and not the whole sentence. I don't think there's a clever way to only highlight a portion of the translated text 🤔
(You picked the one example where it works flawlessly 😄)
I'd suggest just highlighting the whole sentence but this has usability implications beyond i18n. What do you suggest/prefer?

Ahh, thanks for the explanation! It feels weird to add the effect to the whole sentence, see the demo picture below.

I think we have to split the sentence, translate them separately, then do it in the following way:

          println!(
              "\n{} {}\n",
              style(t!("Only")).green().bold(),
              t!("updated repositories will be shown...")
          );

locales/app.yml

SteveLauC · 2024-07-21T14:13:04Z

Hi, I didn't review this today because I was building a locale file checker, it is right there, I have introduced 2 rules by now:

A key should not miss any translation
A key should match its English translation

And it works pretty well, see the errors it catched:

$ wget https://raw.githubusercontent.com/topgrade-rs/topgrade/fe9702330ff9c89c13e7c00e193baaf1e050fa48/locales/app.yml
$ ./target/debug/topgrade_i18n_locale_file_checker app.yml
Errors Found:
  KeyEnMatches
    Path expanded to
    brl list: {output_stdout} {output_stderr}
    Bedrock distribution {distribution}
    Unknown distribution {distribution}
    Checking for gnome extensions: {output}
    Failed to canonicalize {profile_dir}
    Found Nix profile {profile_dir}
    WSL distributions: {wsl_distributions}
    Startup link: {link}

I am thinking about introducing another rule to catch unused keys, tried a little with ast-grep, but haven't figured out how to implement it.

Will review this PR tomorrow:)

SteveLauC · 2024-07-23T05:25:31Z

Cleanup and sort the app.yml to remove duplicated or unneeded keys

This should be done by the locale file checker, I will implement a rule for checking duplicate keys today.

Update: Looks like I don't need to implement this, Yaml, by specification, does not allow duplicate keys in a mapping, and executing the checker against a yaml with duplicate key will panic (the panic comes from serde_yaml):

$ cat app.yaml 
hello:
  en: 1
hello:
  en: 1

$ ./target/debug/topgrade_i18n_locale_file_checker app.yaml
thread 'main' panicked at src/main.rs:25:45:
called `Result::unwrap()` on an `Err` value: Error("duplicate entry with key \"hello\"", line: 1, column: 1)

SteveLauC · 2024-07-23T06:01:36Z

Reviewing, but still have a bunch of files to check😄, will continue this evening.

SteveLauC · 2024-07-23T05:44:22Z

src/command.rs

+            debug!(
+                "{}",
+                t!(
+                    "Command failed: {error_and_context}",


This key Command failed: {error_and_context} and the one Command failed: {command} in line 179/184 are basically same except for the different parameter name, should we consider them as duplicate keys if I implement duplicate key check in the checker?

That's a good question 🤔 On one side they are syntactically the same on the other they differ semantically... I think it would be easier to consider the same keys but with different placeholders as duplicates

I think it would be easier to consider the same keys but with different placeholders as duplicates

Yeah, this is easier to implement since they are literally different 😁

src/error.rs

src/executor.rs

SteveLauC · 2024-07-23T13:38:22Z

I have some thoughts on what should be translated and what should not be, generally, things that are primarily for developers should not be translated:

logs
Panic messages
1. panic!()
2. expect()
3. unwrap_or_else()
Error Contexts
1. context()
2. with_context()
3. wrap_err()
4. wrap_err_with()
Assertion failure message

assert_eq(foo, bar, "foo and bar should have the same value")

And whether a step separator (print_separator()) should be translated depends, some separators can be translated, for example GitHub CLI extension can be easily translated "GitHub 命令行拓展" (Chinese), but things like The ultimate vimrc should really not be translated to "最强的 vimrc" unless that tool officially uses it as the Chinese translation.

Damn, this is sutble, I am not sure if you could feel it...

I realized if we revert the code that should not be translated, that will be a big refactor, I am sorry I didn't realized this at the early stage, it is unfair to let you handle them all, do you mind letting me do some work here?

Mastermindaxe · 2024-07-23T14:16:23Z

@SteveLauC
Hey there! 👋 I'm currently on vacation. I'll try to look at this again over the next few days, but it's unlikely. I don't mind you doing some work on this at all 😄 Thanks for the kind offer. And it's no worries! Let me know if you need more input from me. I'll keep reading the updates, I just won't contribute code over the next few days. I'll happily do more work next week!
Cheers and greetings from Prague

Mastermindaxe · 2024-07-23T14:17:21Z

Cleanup and sort the app.yml to remove duplicated or unneeded keys

This should be done by the locale file checker, I will implement a rule for checking duplicate keys today.

Update: Looks like I don't need to implement this, Yaml, by specification, does not allow duplicate keys in a mapping, and executing the checker against a yaml with duplicate key will panic (the panic comes from serde_yaml):
$ cat app.yaml 
hello:
  en: 1
hello:
  en: 1

$ ./target/debug/topgrade_i18n_locale_file_checker app.yaml
thread 'main' panicked at src/main.rs:25:45:
called `Result::unwrap()` on an `Err` value: Error("duplicate entry with key \"hello\"", line: 1, column: 1)

Correct! That's one advantage with our approach 👍

SteveLauC · 2024-07-23T15:01:13Z

Enjoy your vacation!

Let me handle them, I won't squash commits/force push, so that you can also review them😄

Mastermindaxe · 2024-08-21T11:57:28Z

Hey! @SteveLauC I saw that you were able to make some changes. Give me a shout once it's ready to review. I'd gladly take a look 😊 Excited to bring topgrade to a broader audience! 🚀

SteveLauC · 2024-08-21T13:15:12Z

Give me a shout once it's ready to review. I'd gladly take a look 😊

Yeah, I will 😃

SteveLauC · 2024-09-22T10:45:54Z

Hi @Mastermindaxe, I finished the revert job, you can take a look now. 😪 I am too busy with my work, sorry for the delay.

Once this PR is merged, I plan to release Topgrade v16.0:)

BTW, the CI is not running, I am not quite sure about the reason, I guess it is because we have conflicts in this PR, no need to worry about it.

Mastermindaxe · 2024-09-22T17:30:54Z

Hey @SteveLauC! No worries! 😊 Life can be buddy at times!

I'll check it out once I have some time this week. Looking forward to the release!

Mastermindaxe

LGTM! We will probably see the first "real" bugs once there are translations. Hopefully nothing should break with these changes.

SteveLauC · 2024-09-23T13:44:22Z

Thanks for the review, then I will:

Squash the commits
Rebase the branch
Merge the PR

🚀

SteveLauC · 2024-10-03T10:48:18Z

Merged, thanks for the great work! ❤️

Mastermindaxe · 2024-10-03T11:57:13Z

Merged, thanks for the great work! ❤️

You're welcome! Looking forward to helping out more 😊

Mastermindaxe mentioned this pull request May 19, 2024

add i18n #2

Closed

github-advanced-security bot found potential problems May 19, 2024

View reviewed changes

src/main.rs Fixed Show fixed Hide fixed

SteveLauC mentioned this pull request May 19, 2024

Work queue #738

Closed

Mastermindaxe force-pushed the add_i18n_by_using_rust-i18n branch from aa66d12 to 5f03eb2 Compare May 26, 2024 21:00

Mastermindaxe force-pushed the add_i18n_by_using_rust-i18n branch from 5f03eb2 to a367e99 Compare June 9, 2024 19:32

github-advanced-security bot found potential problems Jun 9, 2024

View reviewed changes

Mastermindaxe force-pushed the add_i18n_by_using_rust-i18n branch from a367e99 to ac8decf Compare June 24, 2024 14:25

Mastermindaxe force-pushed the add_i18n_by_using_rust-i18n branch from 732f29e to 42e91d0 Compare July 1, 2024 13:20

SteveLauC reviewed Jul 6, 2024

View reviewed changes

locales/app.yml Outdated Show resolved Hide resolved

ghost reviewed Jul 7, 2024

View reviewed changes

locales/app.yml Outdated Show resolved Hide resolved

SteveLauC reviewed Jul 23, 2024

View reviewed changes

SteveLauC force-pushed the add_i18n_by_using_rust-i18n branch from fe97023 to ce4106b Compare August 18, 2024 08:53

SteveLauC force-pushed the add_i18n_by_using_rust-i18n branch from bfd3310 to da6f9cb Compare September 22, 2024 09:28

SteveLauC mentioned this pull request Sep 22, 2024

[Feature Request]: Check unused localization keys and the use of keys that do not exist longbridge/rust-i18n#94

Closed

Mastermindaxe commented Sep 23, 2024

View reviewed changes

Mastermindaxe requested a review from SteveLauC September 23, 2024 13:38

SteveLauC force-pushed the add_i18n_by_using_rust-i18n branch from 25fcb19 to cafddc0 Compare October 3, 2024 06:46

feat: initial i18n setup

2c7e8e2

SteveLauC force-pushed the add_i18n_by_using_rust-i18n branch from cafddc0 to 2c7e8e2 Compare October 3, 2024 06:56

SteveLauC added 5 commits October 3, 2024 14:57

style: fmt

e0040f1

feat: i18n support for new steps

9858042

fix: build on Linux

0cc8b9d

fix: build on Linux

76b5852

refactor: rm unused translation keys

0b1aeaf

SteveLauC merged commit 29c555c into topgrade-rs:main Oct 3, 2024
12 checks passed

BrewTestBot mentioned this pull request Oct 6, 2024

topgrade 16.0.0 Homebrew/homebrew-core#192969

Merged

Add i18n by using rust i18n #807

Add i18n by using rust i18n #807

Conversation

Mastermindaxe commented May 19, 2024 • edited Loading

Standards checklist:

SteveLauC commented May 25, 2024

SteveLauC commented May 25, 2024

Mastermindaxe commented May 26, 2024

Mastermindaxe commented May 31, 2024 • edited Loading

SteveLauC commented Jun 1, 2024

Mastermindaxe commented Jun 9, 2024

Mastermindaxe commented Jun 12, 2024

SteveLauC commented Jun 15, 2024

Mastermindaxe commented Jun 24, 2024

Mastermindaxe commented Jun 24, 2024 • edited Loading

Mastermindaxe commented Jun 24, 2024

SteveLauC commented Jun 24, 2024 • edited Loading

SteveLauC commented Jun 25, 2024

Mastermindaxe commented Jun 27, 2024 • edited Loading

Mastermindaxe commented Jun 27, 2024

Mastermindaxe commented Jun 27, 2024

SteveLauC commented Jun 30, 2024

Mastermindaxe commented Jul 1, 2024 • edited Loading

Mastermindaxe commented Jul 4, 2024

SteveLauC commented Jul 6, 2024 • edited Loading

SteveLauC commented Jul 21, 2024

SteveLauC commented Jul 23, 2024 • edited Loading

SteveLauC commented Jul 23, 2024

SteveLauC Jul 23, 2024

Choose a reason for hiding this comment

Mastermindaxe Jul 28, 2024

Choose a reason for hiding this comment

SteveLauC Aug 18, 2024

Choose a reason for hiding this comment

SteveLauC commented Jul 23, 2024

Mastermindaxe commented Jul 23, 2024 • edited Loading

Mastermindaxe commented Jul 23, 2024

SteveLauC commented Jul 23, 2024

Mastermindaxe commented Aug 21, 2024

SteveLauC commented Aug 21, 2024

SteveLauC commented Sep 22, 2024 • edited Loading

Mastermindaxe commented Sep 22, 2024

Mastermindaxe left a comment

Choose a reason for hiding this comment

SteveLauC commented Sep 23, 2024

SteveLauC commented Oct 3, 2024

Mastermindaxe commented Oct 3, 2024

Mastermindaxe commented May 19, 2024 •

edited

Loading

Mastermindaxe commented May 31, 2024 •

edited

Loading

Mastermindaxe commented Jun 24, 2024 •

edited

Loading

SteveLauC commented Jun 24, 2024 •

edited

Loading

Mastermindaxe commented Jun 27, 2024 •

edited

Loading

Mastermindaxe commented Jul 1, 2024 •

edited

Loading

SteveLauC commented Jul 6, 2024 •

edited

Loading

SteveLauC commented Jul 23, 2024 •

edited

Loading

Mastermindaxe commented Jul 23, 2024 •

edited

Loading

SteveLauC commented Sep 22, 2024 •

edited

Loading