feat: localization #279

7ff97222c5

Having them as unit tests meant they would conflict by both trying to
set the same `static`

Cyborus added 1 commit 2026-01-17 21:21:18 +01:00

chore: fmt

All checks were successful

ci/woodpecker/pr/check Pipeline was successful

fix(locale): en-US -> en_US 91ad8c111f

93f330f2df

LordMZTE commented 2026-01-20 21:59:59 +01:00

Collaborator

Copy link

Furthermore, I think users of CLI tools, especially a technical (git-related) CLI tool such as Forgejo-CLI will mostly be people who understand English to a decent degree, so I'm unsure of the impact to user friendliness. We also mustn't forget that a complete translation is impossible anyways since we'd also have to localize flags/subcommands, which I think we agree makes no sense, so the tool still wouldn't be usable without an understanding of English.

Git also has quite a lot of jargon which is just hard to translate in general. As a native German speaker, I can confirm that, when talking about programming topics and especially with tools that invent their own concepts, half the words just remain English as there often is no suitable translation. I think we'll be affected by this phenomenon significantly.

I'm not strictly against this or anything, I just don't quite understand yet :D

I'm curious as to what the motivation is here. I personally find the localization of CLI tools rather problematic since it likes to break scripts and usually offers no real advantage. As an example, if memory serves, apt even localizes Yes/No answers it expects from the user. In German, for example, it expects J instead of Y for a positive answer, breaking simple ad-hoc CLI pipelines like yes | apt .... Furthermore, I think users of CLI tools, especially a technical (git-related) CLI tool such as Forgejo-CLI will mostly be people who understand English to a decent degree, so I'm unsure of the impact to user friendliness. We also mustn't forget that a complete translation is impossible anyways since we'd also have to localize flags/subcommands, which I think we agree makes no sense, so the tool still wouldn't be usable without an understanding of English. Git also has quite a lot of jargon which is just hard to translate in general. As a native German speaker, I can confirm that, when talking about programming topics and especially with tools that invent their own concepts, half the words just remain English as there often is no suitable translation. I think we'll be affected by this phenomenon significantly. I'm not strictly against this or anything, I just don't quite understand yet :D

Cyborus commented 2026-01-21 05:50:02 +01:00

Author

Member

Copy link

Input like this is exactly why I made this a WIP PR so early!

I'm curious as to what the motivation is here

Two main reasons:

I wanted to see if I could! it's an experiment to see how feasible it is.
Localization is important! I want people to be able to work in the language they want, I don't want to force English.

apt even localizes Yes/No answers it expects from the user. In German, for example, it expects J instead of Y for a positive answer, breaking simple ad-hoc CLI pipelines like yes | apt ....

IMO the problem here isn't that apt is localized, but that yes isn't. apt supporting localization seems like decent precedent for us to add it. It seems apt is following libc's advice to localize Y[es]/N[o] to J[a]/N[ein]

...that said, while I think yes should be localized, it still isn't. I'd be open to automatically using LANG=C if a pipe is detected, like how --style minimal does. Or something to accept LANG=C input, but still output in the user's set language.

I think users of CLI tools, especially a technical (git-related) CLI tool such as Forgejo-CLI will mostly be people who understand English to a decent degree, so I'm unsure of the impact to user friendliness.

I don't find this point particularly convincing. If forgejo-cli only supports English, naturally only English-speakers will use it. I have to imagine there are plenty of people who don't know English, or don't know it well, who still use git. Even if they do know English, they may still prefer another language. Having the option is better than not!

Both Forgejo and even git itself support localization, so I think it makes sense for us to as well.

half the words just remain English as there often is no suitable translation. I think we'll be affected by this phenomenon significantly.

Most likely! I'm not sure how much of a problem that would be, though, considering Forgejo and git still do it.

We also mustn't forget that a complete translation is impossible anyways since we'd also have to localize flags/subcommands, which I think we agree makes no sense,

...uh, I was thinking we would, I think clap's alias flag would be able to include a localized version in addition to the English version (Well, the LANG=C version. If I understand it correctly, LANG=C is "whatever the main language of this program is", which is very often english but doesn't have to be. In our case, it is.)

Input like this is exactly why I made this a WIP PR so early! > I'm curious as to what the motivation is here Two main reasons: - I wanted to see if I could! it's an experiment to see how feasible it is. - Localization is important! I want people to be able to work in the language they want, I don't want to force English. > apt even localizes Yes/No answers it expects from the user. In German, for example, it expects J instead of Y for a positive answer, breaking simple ad-hoc CLI pipelines like yes | apt .... IMO the problem here isn't that apt is localized, but that yes *isn't*. apt supporting localization seems like decent precedent for us to add it. It seems apt is following libc's advice to localize Y[es]/N[o] to J[a]/N[ein] ...*that said,* while I think yes *should* be localized, it still *isn't*. I'd be open to automatically using LANG=C if a pipe is detected, like how --style minimal does. Or something to accept LANG=C input, but still output in the user's set language. > I think users of CLI tools, especially a technical (git-related) CLI tool such as Forgejo-CLI will mostly be people who understand English to a decent degree, so I'm unsure of the impact to user friendliness. I don't find this point particularly convincing. If forgejo-cli only supports English, naturally only English-speakers will use it. I have to imagine there are plenty of people who don't know English, or don't know it well, who still use git. Even if they do know English, they may still prefer another language. Having the option is better than not! Both Forgejo and even git itself support localization, so I think it makes sense for us to as well. > half the words just remain English as there often is no suitable translation. I think we'll be affected by this phenomenon significantly. Most likely! I'm not sure how much of a problem that would be, though, considering Forgejo and git still do it. > We also mustn't forget that a complete translation is impossible anyways since we'd also have to localize flags/subcommands, which I think we agree makes no sense, ...uh, I was thinking we would, I *think* clap's alias flag would be able to include a localized version in addition to the English version (Well, the LANG=C version. If I understand it correctly, LANG=C is "whatever the main language of this program is", which is very often english but doesn't have to be. In our case, it is.)

[Screenshot_20260120_173557.png](https://codeberg.org/attachments/63110dee-d8d9-4e73-87e1-5abd100203a6 "Click to see "Screenshot_20260120_173557.png" in a new tab")

52 KiB

LordMZTE commented 2026-01-21 20:19:13 +01:00

Collaborator

Copy link

Your ideas with detecting pipes and falling back to LANG=C does make sense, but CLI flags remain problematic. If we kept the English versions working as you implied by adding aliases, this probably is a non-issue as well.

Some more thoughts:

We should probably include the English flag and subcommand names in all localized help messages. That way, if the user wants to write a script that uses them, they know that they can do so in a language-independent way.
Does clap support runtime-defined aliases? I've not looked into it, but I assumed that alias was an argument to a macro that had to be a string literal.
Why use Fluent? At a first glance, it seems like a very hefty software package. I understand the benefit of it since it allows for "programmable" localization, but why not use a simpler approach directly in Rust, like this:

Those are some good points! I think my main concerns essentially boil down to this: With CLI tools, the line between a user interface and an API is sort of blurry. I agree that localizing a user interface is a good idea, but an API, not so much (as the program using the API would have a hard time doing so correctly in all environments). This is why I had assumed that we don't want to localize flags and subcommands. I figured that bit was clearly more of an API than a user interface, but now, I'm not so sure anymore. Your ideas with detecting pipes and falling back to LANG=C does make sense, but CLI flags remain problematic. If we kept the English versions working as you implied by *adding* aliases, this probably is a non-issue as well. Some more thoughts: - We should probably include the English flag and subcommand names in all localized help messages. That way, if the user wants to write a script that uses them, they know that they can do so in a language-independent way. - Does clap support runtime-defined aliases? I've not looked into it, but I assumed that alias was an argument to a macro that had to be a string literal. - Why use Fluent? At a first glance, it seems like a very hefty software package. I understand the benefit of it since it allows for "programmable" localization, but why not use a simpler approach directly in Rust, like this: rust pub trait Locale { /// The number of comments displayed under a PR. fn num_comments(&self, n: u32) -\> String; } pub struct English; impl Locale for English { fn num_comments(&self, n: u32) -\> String { if n == 1 { "1 comment".to_string(); } else { format!("{n} comments") } } } pub struct German; impl Locale for German { fn num_comments(&self, n: u32) -\> String { if n == 1 { "1 Kommentar".to_string(); } else { format!("{n} Kommentare") } } } where we'd pass around a Box\<dyn Locale\>. In practice, we should probably prefer using the std::fmt::Formatter API to reduce allocations.

Cyborus commented 2026-01-21 21:28:33 +01:00

Author

Member

Copy link

I figured that bit was clearly more of an API than a user interface, but now, I'm not so sure anymore.

Ooh, I do see where you're coming from. That UI vs. API distinction is important but very blurry here.

If we kept the English versions working as you implied by adding aliases, this probably is a non-issue as well.

Yep! That's what I was thinking. And it seems yes, clap does support it. Many of clap's derive macro arguments pass directly through to the corresponding builder function. The only problem I see with that example is that it still shows the English alias even when already using English.

Why use Fluent?

Because it's supported by Weblate. I do also find the idea of a simpler, in-rust solution appealing, but that's hard to make accessible to outside translation contributors. It would limit contributions to only people who know enough Rust.

While brainstorming this, I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included. It might be worth revisiting that idea, so we can have the best of both worlds; the flexibility of Fluent without the runtime concerns. fluent_syntax seems like it would be a great help with that.

> I figured that bit was clearly more of an API than a user interface, but now, I'm not so sure anymore. Ooh, I do see where you're coming from. That UI vs. API distinction is important but very blurry here. > If we kept the English versions working as you implied by _adding_ aliases, this probably is a non-issue as well. Yep! That's what I was thinking. And it seems *yes*, clap does support it. Many of clap's derive macro arguments pass directly through to the corresponding builder function. The only problem I see with that example is that it still shows the English alias even when already using English. > Why use Fluent? Because it's supported by Weblate. I do also find the idea of a simpler, in-rust solution appealing, but that's hard to make accessible to outside translation contributors. It would limit contributions to only people who know enough Rust. While brainstorming this, I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included. It might be worth revisiting that idea, so we can have the best of both worlds; the flexibility of Fluent without the runtime concerns. fluent_syntax seems like it would be a great help with that.

👍 1

xiaoshihou commented 2026-03-30 13:18:22 +02:00

First-time contributor

Copy link

In German, for example, it expects J instead of Y for a positive answer, breaking simple ad-hoc CLI pipelines like yes | apt ....

But again, apt would tell you

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Which fj might or might not apply 🤔

> In German, for example, it expects J instead of Y for a positive answer, breaking simple ad-hoc CLI pipelines like yes | apt .... But again, apt would tell you > WARNING: apt does not have a stable CLI interface. Use with caution in scripts. Which fj might or might not apply 🤔

Cyborus commented 2026-04-05 20:08:05 +02:00

Author

Member

Copy link

@xiaoshihou wrote in #279 (comment):

But again, apt would tell you

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Which fj might or might not apply 🤔

Good thing to point out! I don't think fj is quite to the point where I'd be comfortable guaranteeing a stable CLI interface, but I do aim for it to be usable in scripts. I imagine a lot of things already won't ever be changed.

@Cyborus wrote in #279 (comment):

While brainstorming this, I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included.

This isn't quite as easy as it first appeared — fluent doesn't have clearly defined types! It's ending up with needing to generate functions with lots of generics, which might hurt compile times, but should mean it will be as fast as if we had hard-coded it at runtime.

I haven't finished it yet, but my goal is to get it ready before v0.5.0 so that a call for localization contributions can be included as part of the release.

@xiaoshihou wrote in https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/279#issuecomment-12378183: > But again, apt would tell you > > > WARNING: apt does not have a stable CLI interface. Use with caution in scripts. > > Which fj might or might not apply :thinking: Good thing to point out! I don't think fj is quite to the point where I'd be comfortable *guaranteeing* a stable CLI interface, but I do aim for it to be usable in scripts. I imagine a lot of things already won't ever be changed. --- @Cyborus wrote in https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/279#issuecomment-10051134: > While brainstorming this, I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included. This isn't quite as easy as it first appeared — fluent doesn't have clearly defined types! It's ending up with needing to generate functions with lots of generics, which *might* hurt compile times, but should mean it will be as fast as if we had hard-coded it at runtime. I haven't finished it yet, but my goal is to get it ready before v0.5.0 so that a call for localization contributions can be included as part of the release.

Cyborus commented 2026-04-09 19:38:41 +02:00

Author

Member

Copy link

@Cyborus wrote in #279 (comment):

I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included.

I'm questioning if this is a good idea, just from a maintainability standpoint. Programmatically generating source code is already not easy to keep in my head, but with all the hacks I'm also have to do to get Fluent's data model to fit into Rust's, it's getting really hard to reason about.

I like it as an experiment, and it's possible there's a better way to do it that I'm missing, but for now I think it's too much to do.

I propose we implement localization using at runtime for now so that it can be released and working, then continue work on the compile-time version afterwards, and eventually change to it if it works out.

@Cyborus wrote in https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/279#issuecomment-10051134: > I did have the idea of something in build.rs that generated Rust code from the Fluent files, which could be written to $OUT_DIR and included. I'm questioning if this is a good idea, just from a maintainability standpoint. Programmatically generating source code is already not easy to keep in my head, but with all the hacks I'm also have to do to get Fluent's data model to fit into Rust's, it's getting *really* hard to reason about. I like it as an experiment, and it's possible there's a better way to do it that I'm missing, but for now I think it's too much to do. I propose we implement localization using at runtime *for now* so that it can be released and working, then continue work on the compile-time version afterwards, and eventually change to it if it works out.

Cyborus added 2 commits 2026-04-09 20:27:18 +02:00

test(fluent): placeables in macros

All checks were successful

ci/woodpecker/pr/check Pipeline was successful

033d046f9b

Cyborus commented 2026-04-09 21:04:03 +02:00

Author

Member

Copy link

I've shared my current work on it to the compile-time-fluent branch.

LordMZTE commented 2026-04-09 21:47:36 +02:00

Collaborator

Copy link

@Cyborus wrote in #279 (comment):

I'm questioning if this is a good idea, just from a maintainability standpoint. Programmatically generating source code is already not easy to keep in my head, but with all the hacks I'm also have to do to get Fluent's data model to fit into Rust's, it's getting really hard to reason about.

I'm not exactly sure why we're so dead set on using Fluent in particular. I get that this makes it easier for people to contribute translations, but aren't there other tools offering this that have a less (over-)complicated runtime system?

Another idea I've just had: in case Rust code is hard to generate due to the language constraints (borrow checker and such), it may (or may not) be worth considering the idea of generating C code for the actual buisiness logic (which wouldn't incur any extra dependencies as we already need libc, as Rust does, anyways) as well as some Rust externs. You definitely have a better grasp on the situation when it comes to assessing if this makes sense or not, though :D

@Cyborus wrote in https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/279#issuecomment-12924639: > I'm questioning if this is a good idea, just from a maintainability standpoint. Programmatically generating source code is already not easy to keep in my head, but with all the hacks I'm also have to do to get Fluent's data model to fit into Rust's, it's getting _really_ hard to reason about. I'm not exactly sure why we're so dead set on using Fluent in particular. I get that this makes it easier for people to contribute translations, but aren't there other tools offering this that have a less (over-)complicated runtime system? Another idea I've just had: in case Rust code is hard to generate due to the language constraints (borrow checker and such), it may (or may not) be worth considering the idea of generating C code for the actual buisiness logic (which wouldn't incur any extra dependencies as we already need libc, as Rust does, anyways) as well as some Rust externs. You definitely have a better grasp on the situation when it comes to assessing if this makes sense or not, though :D

Cyborus commented 2026-04-09 22:46:53 +02:00

Author

Member

Copy link

I'm not exactly sure why we're so dead set on using Fluent in particular.

I had picked it because it seems to be the most modern & flexible; it seems to do what it can to minimize the assumptions it makes about how languages work (i.e. how tense, cases, gender, plurals, etc. differ between languages. The intro page of the Fluent wiki describes this issue well). I haven't looked as closely at alternatives, though, so I will spend some time seeing if there's any that are simpler and do that too.

aren't there other tools offering this that have a less (over-)complicated runtime system?

I suspect any localization framework that's flexible enough to accommodate a wide variety of languages is necessarily going to be complex, since natural language is itself complex & varied.

in case Rust code is hard to generate due to the language constraints (borrow checker and such)

It's actually largely not the borrow checker that's getting in the way, it's more that Rust is strongly-typed while Fluent seemingly isn't. Mapping from one to the other is awkward. I imagine adding FFI to that would largely make that more confusing.

> I'm not exactly sure why we're so dead set on using Fluent in particular. I had picked it because it seems to be the most modern & flexible; it seems to do what it can to minimize the assumptions it makes about how languages work (i.e. how tense, cases, gender, plurals, etc. differ between languages. The intro page of the Fluent wiki describes this issue well). I haven't looked as closely at alternatives, though, so I will spend some time seeing if there's any that are simpler *and* do that too. > aren't there other tools offering this that have a less (over-)complicated runtime system? I suspect any localization framework that's flexible enough to accommodate a wide variety of languages is necessarily going to be complex, since natural language is itself complex & varied. > in case Rust code is hard to generate due to the language constraints (borrow checker and such) It's actually largely not the borrow checker that's getting in the way, it's more that Rust is strongly-typed while Fluent seemingly isn't. Mapping from one to the other is awkward. I imagine adding FFI to that would largely make that more confusing.

👍 1

LordMZTE commented 2026-04-09 23:11:28 +02:00

Collaborator

Copy link

it's more that Rust is strongly-typed while Fluent seemingly isn't

Couldn't we simply introduce something like a Value enum here that covers the types that Fluent needs?:

rust

enum Value {
    String(String),
    Integer(u32),
}

it seems to do what it can to minimize the assumptions it makes about how languages work

The most general solution to this issue seems to me like it should just be to allow inline Rust code. For example, in some made-up language mostly inspired by my own template engine as well as Sailfish:

text

# en_US
num_comments n = <% n %> Comment<! if n != 1 { !>s<! } !>

# de_DE
num_comments n = <% n %> Kommentar<! if n != 1 { !>e<! } !>

This should be able to cover all imaginable use cases while also being trivial to implement, cheap performance-wise and relatively contributor-friendly.

> it's more that Rust is strongly-typed while Fluent seemingly isn't Couldn't we simply introduce something like a Value enum here that covers the types that Fluent needs?: rust enum Value { String(String), Integer(u32), } > it seems to do what it can to minimize the assumptions it makes about how languages work The most general solution to this issue seems to me like it should just be to allow inline Rust code. For example, in some made-up language mostly inspired by my own template engine as well as Sailfish: # en_US num_comments n = \<% n %\> Comment\<! if n != 1 { !\>s\<! } !\> # de_DE num_comments n = \<% n %\> Kommentar\<! if n != 1 { !\>e\<! } !\> This should be able to cover all imaginable use cases while also being trivial to implement, cheap performance-wise and relatively contributor-friendly.

Cyborus commented 2026-04-10 00:16:37 +02:00

Author

Member

Copy link

(benchmark run using hyperfine)

With ftl_println!:

text

Benchmark 1: target/release/fj -Hcodeberg.org whoami
  Time (mean ± σ): 33.3 ms ± 2.9 ms [User: 22.6 ms, System: 15.0 ms]
  Range (min … max): 28.1 ms … 49.9 ms 84 runs

With println!:

text

Benchmark 1: target/release/fj -Hcodeberg.org whoami
  Time (mean ± σ): 35.1 ms ± 3.0 ms [User: 25.2 ms, System: 14.0 ms]
  Range (min … max): 28.6 ms … 43.1 ms 77 runs

So, the println one is technically sightly slower. I doubt that's true, rather more likely just random noise, but it shows that the difference between println and ftl_println is negligable, even using Fluent at runtime.

I do welcome you benchmarking it as well if you'd like, to double-check.

Okay, I really should have done this sooner, but I went ahead and benchmarked it. One using Fluent at runtime, and the existing non-localized println call. I did this with the fj whoami command, since that doesn't make a network request, meaning it's the most likely one to have any variance be noticable. (benchmark run using hyperfine) With ftl_println!: Benchmark 1: target/release/fj -Hcodeberg.org whoami Time (mean ± σ): 33.3 ms ± 2.9 ms [User: 22.6 ms, System: 15.0 ms] Range (min … max): 28.1 ms … 49.9 ms 84 runs With println!: Benchmark 1: target/release/fj -Hcodeberg.org whoami Time (mean ± σ): 35.1 ms ± 3.0 ms [User: 25.2 ms, System: 14.0 ms] Range (min … max): 28.6 ms … 43.1 ms 77 runs So, the println one is technically sightly slower. I doubt that's true, rather more likely just random noise, but it shows that the difference between println and ftl_println is negligable, even using Fluent at runtime. I do welcome you benchmarking it as well if you'd like, to double-check.

Cyborus commented 2026-04-15 21:26:18 +02:00

Author

Member

Copy link

Few updates:

@LordMZTE and I discussed this further in DMs and we agreed:
1. that the above benchmark wasn't of much help,
2. but using Fluent at runtime is still probably fine.
However, it seems I won't be able to merge this before the next release (tomorrow!!), so...
My plan is to still have an announcement for this in the next release notes as an upcoming feature, and to get it merged shortly after release. That way, there will be an entire release cycle of time to get translation contributions, so that when the feature does launch as part of v0.6.0, it will actually be useful!

Few updates: - @LordMZTE and I discussed this further in DMs and we agreed: 1. that the above benchmark wasn't of much help, 2. but using Fluent at runtime is still probably fine. - However, it seems I won't be able to merge this before the next release (tomorrow!!), so... - My plan is to still have an announcement for this in the next release notes *as an upcoming feature*, and to get it merged shortly *after* release. That way, there will be an entire release cycle of time to get translation contributions, so that when the feature does launch as part of v0.6.0, it will actually be useful!

fogti commented 2026-04-17 00:39:29 +02:00

First-time contributor

Copy link

Maybe one should look at how rustc implements translations / localization, given that at least some parts of it also use fluent.

Cyborus commented 2026-04-17 00:46:06 +02:00

Author

Member

Copy link

I was not aware rustc implemented localization! Though from a quick search, it seems to be in a rather incomplete and uncertain state: https://github.com/rust-lang/rust/issues/132181

alexandresoro referenced this pull request from a commit 2026-04-19 14:37:34 +02:00 chore(deps): update dependency forgejo-contrib/forgejo-cli to v0.5.0 (#1003)

Cyborus force-pushed localization from 033d046f9b

All checks were successful

ci/woodpecker/pr/check Pipeline was successful

to 0d30e210f5

Some checks failed

ci/woodpecker/pr/check-typos Pipeline failed

ci/woodpecker/pr/check Pipeline was successful

2026-05-12 22:36:38 +02:00 Compare

Cyborus force-pushed localization from 0d30e210f5

Some checks failed

ci/woodpecker/pr/check-typos Pipeline failed

ci/woodpecker/pr/check Pipeline was successful

to 65dd1120e9

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

2026-05-12 22:38:56 +02:00 Compare

Cyborus force-pushed localization from c738e0f20a

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

to e7a7c89ca2

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

2026-05-12 23:04:19 +02:00 Compare

Cyborus force-pushed localization from e7a7c89ca2

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

to 4ca9bf43fc

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

2026-05-12 23:10:43 +02:00 Compare

Cyborus force-pushed localization from 4ca9bf43fc

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

to 3473476889

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

2026-05-13 00:03:55 +02:00 Compare

Cyborus force-pushed localization from 3473476889

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

to cfc80f569b

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

2026-05-13 00:27:18 +02:00 Compare

fogti reviewed 2026-05-13 00:31:48 +02:00

Cargo.toml Outdated

Show resolved Hide resolved

fogti commented 2026-05-13 00:31:48 +02:00

First-time contributor

Copy link

This doesn't look like it is necessary, given that build.rs doesn't appear to use eyre.

Cyborus commented 2026-05-13 00:38:55 +02:00

Author

Member

Copy link

Thanks! That was a leftover from an earlier version.

Cyborus marked this conversation as resolved

Cyborus added 1 commit 2026-05-13 00:39:25 +02:00

chore: remove eyre from build-dependencies

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

3526e0b9a0

Cyborus commented 2026-05-13 00:45:19 +02:00

Author

Member

Copy link

@LordMZTE I get if you choose not to review this, but if you do, most of actual new code is in src/localization.rs. Almost all the other changes are just localizing the rest of the project

I believe this is now, finally, ready to be merged. I had talked about wanting to localize flag/subcommand names & help messages, and I still would, but those can come as their own PRs I think. I'd rather not delay this any further. @LordMZTE I get if you choose not to review this, but if you do, most of actual new code is in src/localization.rs. Almost all the other changes are just localizing the rest of the project

Cyborus requested review from LordMZTE 2026-05-13 00:45:30 +02:00

Cyborus changed title from ** ~~WIP: feat: localization~~ ** to feat: localization 2026-05-13 00:45:39 +02:00

LordMZTE requested changes 2026-05-13 15:52:19 +02:00 Dismissed

LordMZTE left a comment

Copy link

Looking great!

Unfortunately, Codeberg did the thing again where some of the review comments move to wrong lines randomly, so I apologize if they're out of place.

Looking great! Unfortunately, Codeberg did the thing again where some of the review comments move to wrong lines randomly, so I apologize if they're out of place.

build.rs Outdated

Show resolved Hide resolved

| | | | @ -4,2 +4,4 @@ | | | | | | std::env::var("TARGET").unwrap() | | | | | | ); | | | | | | | | | | | | for path in glob::glob("localization/*/*.ftl").unwrap() { |

LordMZTE commented 2026-05-13 14:49:53 +02:00

Collaborator

Copy link

Is this really worth introducing a dependency on glob? Should be easy enough to implement this without it.

Cyborus marked this conversation as resolved

build.rs

Show resolved Hide resolved

| | | | @ -6,0 +19,4 @@ | | | | | | } | | | | | | }; | | | | | | if let Err((_, errs)) = fluent_syntax::parser::parse(file.as_str()) { | | | | | | let pre = "cargo::warning="; |

LordMZTE commented 2026-05-13 14:47:52 +02:00

Collaborator

Copy link

Why only emit a warning here and not an error?

Cyborus marked this conversation as resolved

localization/en-US/messages.ftl Outdated

Show resolved Hide resolved

| | | | @ -0,0 +15,4 @@ | | | | | | msg-auth-login-browser_success = Authenticated! Close this tab and head back to your terminal. | | | | | | msg-auth-login-browser_failure = Failed to authenticate. | | | | | | | | | | | | msg-auth_logout-success = signed out of {$username}@{$host} |

LordMZTE commented 2026-05-13 14:53:10 +02:00

Collaborator

Copy link

We seem to be quite inconsistent with whether messages start with an upper-case or lower-case letter. I think it'd be good to agree on one convention here. Similarly, we also sometimes have periods at the end of messages and sometimes don't.

Cyborus commented 2026-05-13 18:01:36 +02:00

Author

Member

Copy link

I agree, but that seems out-of-scope to me.

LordMZTE marked this conversation as resolved

localization/en-US/messages.ftl Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:02:24 +02:00

Collaborator

Copy link

This seems like such a general message that it might make sense to change the key to not be agit-specific.

Cyborus commented 2026-05-13 17:58:39 +02:00

Author

Member

Copy link

I think it's quite likely that if the user isn't creating the PR with AGit, then they've already pushed their branch with git and it's set up the remote tracking.

LordMZTE commented 2026-05-13 22:20:05 +02:00

Collaborator

Copy link

I was referring to generalizing the concept of "Yes/No/What?", but as you elaborated in another comment, this doesn't work for all languages anyways.

👍 1

LordMZTE marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -0,0 +61,4 @@ | | | | | | } | | | | | | | | | | | | msg-org-create-invalid_character = | | | | | | Organization names can only have alphanumeric characters, dash, underscore, or period. |

LordMZTE commented 2026-05-13 14:56:12 +02:00

Collaborator

Copy link

It seems to me like we should use the plural of character names here, i.e. "dashes" rather than "dash". That feels more natural. I'd also likely write "and periods" instead of "or periods", but that's debatable.

👍 1

Cyborus marked this conversation as resolved

localization/en-US/messages.ftl Outdated

Show resolved Hide resolved

| | | | @ -0,0 +416,4 @@ | | | | | | | | | | | | msg-repo-unstar-success = Removed star from {$owner}/{$repo}! | | | | | | | | | | | | msg-repo-delete-confirmation_prompt = Are you sure you want to delete {$owner}/{$name}? (y/N) |

LordMZTE commented 2026-05-13 15:06:45 +02:00

Collaborator

Copy link

We could have one localized message with the actual prompt, and one with just "(y/N)", allowing us to re-use the latter, which we seem to duplicate quite a bit.

Cyborus commented 2026-05-13 17:48:23 +02:00

Author

Member

Copy link

The duplication here is actually entirely intentional. As I understand it, many languages (such as Mandarin) answer yes/no questions by repeating the verb instead of using a word for "Yes", so the answer here would be something like "Want" or "Delete". So the confirmation option in each prompt would be different depending on the question being asked.

LordMZTE marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -0,0 +88,4 @@ | | | | | | [some] {STYLE("bold", "bright-cyan")}{$full_name}{STYLE("reset")} {STYLE("light-gray")}({$username}){STYLE("reset")} | | | | | | } | | | | | | | | | | | | msg-org-visibility-public = You are a public member of {STYLE("bold", "bright-cyan")}{$org_name}{STYLE("reset")} |

LordMZTE commented 2026-05-13 14:58:45 +02:00

Collaborator

Copy link

These feel duplicated. Could we perhaps come up with some solution to parametrize the messages (for the access levels and "set" vs "get") so that this is more concise?

Cyborus commented 2026-05-13 18:17:08 +02:00

Author

Member

Copy link

IMO having them separate makes the code simpler to read, taking each variation as a parameter sounds kinda awkward.

I think it's better to be a bit more lax on duplication in localization than in the code, too.

IMO having them separate makes the code simpler to read, taking each variation as a parameter sounds kinda awkward. I think it's better to be a bit more lax on duplication in localization than in the code, too.

LordMZTE marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -0,0 +533,4 @@ | | | | | | msg-activity-commented_pr = {STYLE("bold")}{$actor}{STYLE("reset")} commented on pull request {STYLE("bold", "yellow")}{$repo_name}#{$number}{STYLE("reset")} | | | | | | msg-activity-created_release = {STYLE("bold")}{$actor}{STYLE("reset")} created release {STYLE("bold", "bright_cyan")}{$release_name}{STYLE("reset")} on {STYLE("bold", "yellow")}{$repo_name}{STYLE("reset")} | | | | | | | | | | | | msg-user-edit-name-removal_hint = Use --unset to remove your name from your profile |

LordMZTE commented 2026-05-13 15:08:29 +02:00

Collaborator

Copy link

Seems like this could be deduplicated.

Cyborus commented 2026-05-13 18:17:25 +02:00

Author

Member

Copy link

Same reasoning as #279 (comment)

Same reasoning as https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/279#issuecomment-14892255

LordMZTE marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -0,0 +593,4 @@ | | | | | | [yes] {STYLE("bright-cyan")}true{STYLE("reset")} | | | | | | *[no] {STYLE("bright-cyan")}false{STYLE("reset")} | | | | | | } | | | | | | msg-user-gpg-list-can_encrypt_comms = {STYLE("bold")}Can Encrypt Comms:{STYLE("reset")} {$can_encrypt_comms -> |

LordMZTE commented 2026-05-13 15:11:17 +02:00

Collaborator

Copy link

Could we pull the STYLE-directives out of this case distinction here? Maybe we can even re-use the formatting of boolean values across multiple messages?

Cyborus commented 2026-05-13 17:54:17 +02:00

Author

Member

Copy link

Uh, whoops, those aren't supposed to be cyan. They're supposed to be green for true and red for false, like they are in main.

Cyborus marked this conversation as resolved

src/actions.rs

Show resolved Hide resolved

| | | | @ -281,3 +282,3 @@ | | | | | | })) => { | | | | | | if !force { | | | | | | bail!("variable already exists, pass --force to replace it."); | | | | | | ftl_bail!("msg-actions-variable-create-already_exists"); |

LordMZTE commented 2026-05-13 14:51:27 +02:00

Collaborator

Copy link

Why do we mix underscores and dashes here (as well as in many other localization keys)?

Cyborus commented 2026-05-13 17:51:47 +02:00

Author

Member

Copy link

I use the - as a hierarchy connection (like / in a file system or . in accessing struct fields), and _ as a space within a single segment.

LordMZTE commented 2026-05-13 22:20:42 +02:00

Collaborator

Copy link

I think it'd make sense to document this somewhere - it seemed arbitrary to me at first glance.

👍 2

Cyborus commented 2026-05-17 17:42:28 +02:00

Author

Member

Copy link

Seems like something that could go in a CONTRIBUTING.md, which will be my next major project here

LordMZTE marked this conversation as resolved

src/auth.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:15:46 +02:00

Collaborator

Copy link

This seems like it'd be reachable by the client setting only the code or state query parameter. Am I missing something?

Cyborus commented 2026-05-13 18:20:40 +02:00

Author

Member

Copy link

In theory yeah, but I wasn't able to find a way to make it do that. Forgejo seems to always return them together. Saves me having to localize the "Canceled" message, too, which is why I did it as part of this PR.

LordMZTE commented 2026-05-13 22:28:23 +02:00

Collaborator

Copy link

Sure, but consider that:

Forgejo can change it's behavior arbitrarily without notice, and so can instance admins, and
we have no guarantee that Forgejo is the one specifying the URL, it could also be the user messing about and forging requests.

Given that unreachable!() should be used to indicate that there's provably no way the code can be executed under any circumstances (in fact, in most languages unreachable also acts as a hint to the optimizer so it can assume a condition and use that to its advantage - this isn't done in Rust since this requires the unreachable statement to invoke undefined behavior), it's not the right choice here. However, given that this branch is unlikely to be reached without it being deliberately done by the user, I see no need to localize the error message here.

Generally, I think it's fine to leave error messages that are unlikely to ever be printed out (which I've found to be surprisingly quite common) unlocalized.

Sure, but consider that: - Forgejo can change it's behavior arbitrarily without notice, and so can instance admins, and - we have no guarantee that Forgejo is the one specifying the URL, it could also be the user messing about and forging requests. Given that unreachable!() should be used to indicate that there's provably no way the code can be executed under any circumstances (in fact, in most languages unreachable also acts as a hint to the optimizer so it can assume a condition and use that to its advantage - this isn't done in Rust since this requires the unreachable statement to invoke undefined behavior), it's not the right choice here. However, given that this branch is unlikely to be reached without it being deliberately done by the user, I see no need to localize the error message here. Generally, I think it's fine to leave error messages that are unlikely to ever be printed out (which I've found to be surprisingly quite common) unlocalized.

👍 2

Cyborus commented 2026-05-13 23:04:16 +02:00

Author

Member

Copy link

(which I've found to be surprisingly quite common)

Especially in this codebase, aha, look at all the "field not set" messages.

I think this can probably even just return an empty string. I really doubt Forgejo will ever trip it, I'd be really concerned if it stopped sending either of the needed fields.

> (which I've found to be surprisingly quite common) Especially in this codebase, aha, look at all the "field not set" messages. I think this can probably even just return an empty string. I really doubt Forgejo will ever trip it, I'd be really concerned if it stopped sending either of the needed fields.

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:16:45 +02:00

Collaborator

Copy link

Nitpick: a trailing comma would look nice here. :D

See my comments at the declaration for how to support this with macros.

Nitpick: a trailing comma would look nice here. :D See my comments at the declaration for how to support this with macros.

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:17:10 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:17:15 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:19:49 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

| | | | @ -727,1 +702,3 @@ | | | | | | println!("({} attachments)", assets.len()); | | | | | | ftl_println!( | | | | | | "msg-issue-view-comments-attachments", | | | | | | attachments = assets.len() |

LordMZTE commented 2026-05-13 15:20:03 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:20:24 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:20:34 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/issues.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:20:43 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +18,4 @@ | | | | | | bundle.add_function("STYLE", style).unwrap(); | | | | | | bundle.add_function("IS_MINIMAL", is_minimal).unwrap(); | | | | | | bundle.add_function("OPT", opt).unwrap(); | | | | | | // .ftl files are checked at compile time inbuild.rs |

LordMZTE commented 2026-05-13 15:22:23 +02:00

Collaborator

Copy link

Well, they're checked for syntax errors, but are we sure that's all that could possibly happen here? Are we covering all error cases in our check in build.rs?

Cyborus commented 2026-05-13 18:44:37 +02:00

Author

Member

Copy link

Yes, FluentResource::try_new returns Vec<ParserError> as its error type, directly from fluent_syntax.

Yes, FluentResource::try_new returns Vec\<ParserError\> as its error type, directly from fluent_syntax.

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +211,4 @@ | | | | | | ($msg_id:expr) => { | | | | | | $crate::ftl_format!($msg_id,) | | | | | | }; | | | | | | ($msg_id:expr, $($var_name:ident $(= $var_val:expr)?),*) => { |

LordMZTE commented 2026-05-13 15:30:17 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +281,4 @@ | | | | | | | | | | | | pub struct WriterCompat<W>(pub W); | | | | | | | | | | | | impl<W: std::io::Write> std::fmt::Write for WriterCompat<W> { |

LordMZTE commented 2026-05-13 15:40:13 +02:00

Collaborator

Copy link

This should probably also provide an implementation of write_fmt that delegates to std::io::Write::write_fmt so we can utilize a potential efficient implementation in the underlying writer. It also seems strange that this struct is even needed, but I also can't really find an alternative to it.

👍 1

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +178,4 @@ | | | | | | fluent_bundle::FluentArgs::new() | | | | | | } | | | | | | }; | | | | | | ($($var_name:ident $(= $var_val:expr)?),*) => { |

LordMZTE commented 2026-05-13 15:29:50 +02:00

Collaborator

Copy link

Should have $(,)? to suppor trailing commas.

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +292,4 @@ | | | | | | ($msg_id:expr) => { | | | | | | $crate::ftl_print!($msg_id,) | | | | | | }; | | | | | | ($msg_id:expr, $($var_name:ident $(= $var_val:expr)?),*) => { |

LordMZTE commented 2026-05-13 15:40:27 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/main.rs Outdated

Show resolved Hide resolved

| | | | @ -291,17 +310,17 @@ struct SpecialRender { | | | | | | horiz_rule: char, | | | | | | | | | | | | // Uncomment these as needed |

LordMZTE commented 2026-05-13 15:46:20 +02:00

Collaborator

Copy link

We can probably remove this comment since they're now all uncommented.

👍 1

Cyborus marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -612,1 +579,3 @@ | | | | | | println!("Created new label {}", crate::render_label(&label)?); | | | | | | ftl_println!( | | | | | | "msg-org-label-add-success", | | | | | | label = crate::render_label(&label)? |

LordMZTE commented 2026-05-13 15:48:03 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:48:11 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

Show resolved Hide resolved

| | | | @ -658,1 +628,3 @@ | | | | | | println!("Removed label {}", crate::render_label(&label)?); | | | | | | ftl_println!( | | | | | | "msg-org-label-remove-success", | | | | | | label = crate::render_label(&label)? |

LordMZTE commented 2026-05-13 15:48:23 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

refactor: remove build-dependency on glob c60f3ecadb

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:48:31 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/org/team.rs Outdated

Show resolved Hide resolved

| | | | @ -385,0 +365,4 @@ | | | | | | "msg-org-team-create-success", | | | | | | name, | | | | | | org = org_name, | | | | | | admin = if flags.admin { "yes" } else { "no" } |

LordMZTE commented 2026-05-13 15:49:02 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/org/team.rs Outdated

Show resolved Hide resolved

| | | | @ -426,2 +409,2 @@ | | | | | | let confirmation = crate::readline("(y/N) ").await?.to_lowercase(); | | | | | | if matches!(confirmation.trim(), "y" | "yes") { | | | | | | let confirmation = ftl_prompt_bool!( | | | | | | default false; "msg-org-team-delete-confirmation", org = &org, name = &name |

LordMZTE commented 2026-05-13 15:49:21 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/org/team.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:49:30 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/org/team.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:49:45 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/release.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:50:11 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/release.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:50:20 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/release.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:50:29 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

src/wiki.rs Outdated

Show resolved Hide resolved

LordMZTE commented 2026-05-13 15:51:26 +02:00

Collaborator

Copy link

Ditto

Cyborus marked this conversation as resolved

Cyborus added 8 commits 2026-05-13 18:38:24 +02:00

fix(string): bool colors in user-gpg-list-*... cac402d1fd

They were incorrectly both `bright-cyan` when they
should have been `bright-green` and `bright-red`.

build: display ftl errors as errors instead of warnings 5640d3dbaa

build: rerun if localization is changed 2b6671dcea

refactor: allow trailing comma in ftl_* macros 8284247a05

fix(string): consistent pluralization in org-create-invalid_character b7192ce8d7

chore: remove obsolete comment ac9bbb9180

refactor: impl AsFluent for bool

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

feat: passthrough write_fmt on WriterCompat

5183b942a1

Cyborus added 1 commit 2026-05-13 18:45:57 +02:00

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

fix: remove unreachable from auth::auth_server

3ad20eb540

Cyborus requested review from LordMZTE 2026-05-13 18:47:02 +02:00

Cyborus added 1 commit 2026-05-17 17:43:02 +02:00

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

fix(ftl): variable interpolation syntax... 57346f0d11

1c1228ed7f

LordMZTE requested changes 2026-05-17 21:19:41 +02:00 Dismissed

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +76,4 @@ | | | | | | let mut out = String::new(); | | | | | | for arg in positional { | | | | | | let FluentValue::String(style) = arg else { | | | | | | continue; |

LordMZTE commented 2026-05-17 20:41:20 +02:00

Collaborator

Copy link

Shouldn't we return an error here? Seems like silently ignoring invalid arguments isn't that great of a solution.

Cyborus marked this conversation as resolved

src/localization.rs

Show resolved Hide resolved

| | | | @ -0,0 +113,4 @@ | | | | | | } | | | | | | | | | | | | pub fn is_minimal<'a>(_: &[FluentValue<'a>], _: &FluentArgs<'_>) -> FluentValue<'a> { | | | | | | if crate::special_render().fancy { |

LordMZTE commented 2026-05-17 20:48:51 +02:00

Collaborator

Copy link

This can probably be simplified to crate::special_render().fancy.ftl().into().

👍 1

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +121,4 @@ | | | | | | } | | | | | | | | | | | | pub fn opt<'a>(positional: &[FluentValue<'a>], _: &FluentArgs<'_>) -> FluentValue<'a> { | | | | | | let Some(value) = positional.first() else { |

LordMZTE commented 2026-05-17 20:46:10 +02:00

Collaborator

Copy link

We should probably also fail here if we have too many arguments:

rust

// Fail on extra arguments
let [value] = positional else { return FluentValue::Error; };

In any case, this can be simplified:

rust

// Equivalent to current code
let [value, ..] = positional else { return FluentValue::Error; };

We should probably also fail here if we have too many arguments: rust // Fail on extra arguments let [value] = positional else { return FluentValue::Error; }; In any case, this can be simplified: rust // Equivalent to current code let [value, ..] = positional else { return FluentValue::Error; };

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +143,4 @@ | | | | | | fn ftl(self) -> Self::FluentType { | | | | | | let date = icu_calendar::Date::try_new_iso(self.year(), self.month() as u8, self.day()) | | | | | | .expect( | | | | | | "icu_calendar::Date and time::Date share the same range restraints so this can't panic", |

LordMZTE commented 2026-05-17 20:51:52 +02:00

Collaborator

Copy link

Nitpick: I think the so this can't panic suffix should be dropped. That way, the user won't see

text

thread 'main' panicked [...]:
[...] this can't panic

...in case it ever does panic for whatever reason :P

Nitpick: I think the so this can't panic suffix should be dropped. That way, the user won't see thread 'main' panicked [...]: [...] this can't panic ...in case it ever does panic for whatever reason :P

👍 1

Cyborus marked this conversation as resolved

src/localization.rs Outdated

Show resolved Hide resolved

| | | | @ -0,0 +206,4 @@ | | | | | | ($msg_id:expr) => {{ | | | | | | $crate::localization::bundles::locale() | | | | | | .into_iter() | | | | | | .filter_map(|b| Some((b, b.get_message($msg_id)?))) |

LordMZTE commented 2026-05-17 21:05:25 +02:00

Collaborator

Copy link

.filter_map(...).first() can be simplified to .find(...). Note that Clippy informed me of this, we might want to check this as part of the CI.

Cyborus commented 2026-05-17 21:47:18 +02:00

Author

Member

Copy link

I think that might be a false positive in Clippy? .find(...) can replace .filter(...).next(), but it doesn't include the .map(...) part. It only takes a bool instead of Option<T>

I think that might be a false positive in Clippy? .find(...) can replace .filter(...).next(), but it doesn't include the .map(...) part. It only takes a bool instead of Option\<T\>

Cyborus marked this conversation as resolved

src/main.rs Outdated

Show resolved Hide resolved

| | | | @ -118,0 +117,4 @@ | | | | | | ) -> eyre::Result<Option<&'static str>> { | | | | | | use std::io::Write; | | | | | | let (bundle, message) = | | | | | | ftl_message!(msg_id).ok_or_eyre("cannot prompt for bool: invalid fluent message id")?; |

LordMZTE commented 2026-05-17 20:53:45 +02:00

Collaborator

Copy link

I think it'd make more sense for this to be an expect, as it's programmer error and we also panic later on if we fail to format the message.

👍 1

Cyborus marked this conversation as resolved

src/main.rs Outdated

Show resolved Hide resolved

| | | | @ -118,3 +138,2 @@ | | | | | | } else { | | | | | | format!("{msg} [y/N]: ") | | | | | | }; | | | | | | write!(&mut stdout, "{}", std::borrow::Cow::from(msg_id))?; |

LordMZTE commented 2026-05-17 21:02:22 +02:00

Collaborator

Copy link

Why do we need to turn msg_id into a Cow here? Also, is printing the localization key really the right solution here if we have a message wihout a value set (I don't even know how that can happen)?

Cyborus commented 2026-05-17 21:50:08 +02:00

Author

Member

Copy link

I'm not sure how that happened? My only guess is I copied it from ftl_format, but that's strange since it acts more like ftl_write. Nevertheless, it can probably just skip printing anything

Cyborus marked this conversation as resolved

Cyborus added 7 commits 2026-05-17 22:38:15 +02:00

A few places were missing the required `$` before
the variable name.

fix(fluent): stricter argument validation 132fb25bee

refactor(fluent): simpler IS_MINIMAL body e13e091850

refactor: better panic message in OffsetDateTime::ftl 89b3276749

fix(l10n): panic on invalid user prompt message 13fa2c8e1b

fix(l10n): don't print message id on empty prompt message 0f61c26fa0

fix(ftl): minor fixes

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

Cyborus referenced this pull request 2026-05-17 22:40:40 +02:00 better error printing #112

9942283ef3

Cyborus added 1 commit 2026-05-17 22:49:16 +02:00

refactor: .filter_map().next() -> .find_map()

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful

fix(l10n): bad en-US messages 66f164d54b

8b2dcf45a9

Cyborus requested review from LordMZTE 2026-05-17 22:53:06 +02:00

LordMZTE approved these changes 2026-05-18 08:10:17 +02:00 Dismissed

Cyborus added 2 commits 2026-05-18 15:06:25 +02:00

chore(typos): exclude locales from spell check

All checks were successful

ci/woodpecker/pr/check-typos Pipeline was successful

ci/woodpecker/pr/check Pipeline was successful