Skip to content

Error rewrite#12556

Closed
Nahor wants to merge 3 commits into
fish-shell:masterfrom
Nahor:error_rewrite
Closed

Error rewrite#12556
Nahor wants to merge 3 commits into
fish-shell:masterfrom
Nahor:error_rewrite

Conversation

@Nahor
Copy link
Copy Markdown
Contributor

@Nahor Nahor commented Mar 17, 2026

Partial implementation of my idea for a more homogenized error printing.
Currently, only set_color and the builtin shared helpers have been ported.

All builtins and main have been updated.

Please comment.

The change in the localization is because I removed the "%s" for the command from the error message in favor of a "%s{cmd}: %s{msg}", cmd, fmt!(msg, args) kind of thing (and similarly with a %s{cmd}: %s{subcmd}: %s{msg} for subcommands)

I'm hesitating to add with_option(wstr)+with_options(wstr,wstr). The main concern here is either a combinatorial explosion in print_msg (for a total of 9 branches), or the deep-ish nesting of wgettext_fmt! calls (cmd/subcmd > options > msg > who knows how many to construct msg)

TODOs:

  • If addressing an issue, a commit message mentions Fixes issue #<issue-number>
  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@danielrainer
Copy link
Copy Markdown

I like the idea of a more unified error handling approach.

There is one thing worth considering: We've been trying to split up the main library crate. So far, only a bit of code has been extracted into crates/, but the idea is to incrementally shrink the main library crate. The primary motivation is improving incremental build performance, but eliminating dependency cycles, which is necessary for such extraction to work, should also make the codebase cleaner overall. Functionality like what's proposed in this PR will likely be depended on in many places, so ideally it would already be in its own crate. Right now, this isn't possible, because it depends on several things from the main crate, namely gettext stuff, io::IoStreams, and parser::Parser. I'll ignore the gettext stuff for now, since it can be replaced by Fluent once that PR is ready, or otherwise we'll extract gettext. Depending on io::IoStreams could be avoided by making the printing functions return a WString. That would require more allocations and more verbose handling at the call sites, so it's not ideal. Alternatively, we might be able to extract the IoStreams definition such that it can be depended on outside of the main crate. Parser would be fairly easy to replace, by changing with_stacktrace to take the stacktrace directly (as generated by parser.current_line()). The name might be a bit misleading, since it doesn't really show a stacktrace, only the line on which the error occurred. I'm not saying that the extraction related changes should happen immediately, since there it no real point in applying them while localization is only available in the main crate.

Comment thread src/error.rs Outdated
"%s: missing man page\nDocumentation may not be installed.\n`help %s` will show an online version"

/// Error message on multiple scope levels for variables.
pub GLOCAL
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks like a typo. It's probably intended to combine global and local, but that's not obvious from the name alone, it does not account for the universal and function scopes, and doesn't communicate what's wrong. Maybe we can call it something like MULTIPLE_SCOPES. At the moment, this constant seems unused while BUILTIN_ERR_GLOCAL in src/builtins/shared.rs is still used.

I know that this isn't your naming choice. The name has been in the codebase for a while and I've also touched it before, but I only realized this naming now, so I suggest we improve it, although that can happen independently of this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At the moment, this constant seems unused while BUILTIN_ERR_GLOCAL in src/builtins/shared.rs is still used.

This PR is just an RFC for now. So Error contains all the values from shared.rs. Nearly all of them are unused right now. Some still contain the %s for a cmd/subcmd.

That said, I'm changing my approach to this PR. I'm doing a first pass/commit to standardize the messages themselves (e.g. extract the cmd/subcmd from the error msg, using nested append() instead). This includes revisiting the constant names among other things (e.g. MISSING should be MISSING_OPT_ARG, and UNEXP_ARG should be UNEXP_OPT_ARG... well, their BUILTIN_ERR_xxx equivalent in shared.rs).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Improving the constant names seems like a good start.

Generally, having localized messages which are as complete as possible is desirable, since it gives most context and flexibility to translators. With the command, and in some cases subcommand, prefixes, I think it's mostly fine to split it up and have nested wgettext_fmt! calls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That said, I'm changing my approach to this PR. I'm doing a first pass/commit to standardize the messages themselves

Scratch that. This is just too redundant. Too many &wgettext_fmt!("%s: %s", ..., &wgettext_fmt!(...)) added for every messages. And constants need to be temporarily doubled (one with the "%s: " prefix, one without) until all call sites get updated. I can keep the renaming separate, but the splitting (and other tweaks, like lower-casing the initial letter) might as well be done while switching to Error.

Comment thread src/error.rs Outdated
.err
.appendln(&wgettext_fmt!("%s: %s: %s", cmd, subcmd, &self.msg));
}
(None, Some(_)) => unreachable!("cannot have `subcmd` without `cmd`"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We could make this unrepresentable by changing the definition of Error such that cmd has type Option<(&'a wstr, Option<&'a wstr>)>, and subcmd no longer exists. Makes the definition harder to read, so it probably deserves a comment, but it prevents having a subcommand without a command by construction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I dislike this unreachable! as well.

At the same time Some(cmd, Some(subcmd)) (or let Some(cmd, _) = self.cmd below) starts to be unwieldy/awkward, especially given that subcmd without cmd is not publicly constructible.

Alternatively, it would be to replace (None, None) with (None, _) above, which makes some sense since subcmd without cmd could be seen as meaningless rather than an error.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, it's a tradeoff. Matching on (None, _) might be a decent approach. It would hide incorrectly constructed Errors, but the interface is fairly straightforward, so I think the risk of the code being changed to allow such a construction is acceptably low.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe

struct CommandName<'a> {
    cmd: &'a wstr,
    subcmd: Option<&'a wstr>,
}

and then command_name: Option<CommandName<'a>>

@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Mar 21, 2026

I've updated 1/3 of the builtins (abbr-fg).
Unlike what I was planning to do initially, I'm minimizing the updates to the strings for now (just removing the "%s" prefixes) because it's easier to track the changes in the po files. And also, I'm not sure how much the error message are part of the fish API.

@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Mar 23, 2026

All builtins have been converted. I'll see about improving the test coverage for errors before calling it "done" though.

EDIT: for the record, there are 142 error sites (where err_xxx!(...) is called) that are triggered by one test or another. There are 185 that are never triggered.

@Nahor Nahor changed the title RFC: error rewrite Error rewrite Mar 23, 2026
@krobelus
Copy link
Copy Markdown
Contributor

krobelus commented Mar 25, 2026

Good cleanup.
I wouldn't mind squashing most commits, no strong opinion.
I hope I didn't miss anything major in my review.

-# CHECKERR: string repeat: Invalid count value '-1'
+# CHECKERR: string: repeat: Invalid count value '-1'

I like the old formatting (without the colon) better, it seems more natural.

+        err_fmt!("Unexpected argument -- '%s'", &opts.args[0])
+            .with_subcmd(CMD, subcmd)
+            .finish(streams);

I think streams.append(err_fmt!(...)) would be a natural invocation,
because that's what we already do for strings.
Not sure if we want to use that exact name but we could (may need to define a trait).
I guess a method on error is more discoverable, though it's unclear if that matters.
Maybe we should rename "finish" to "write_to" or similar.
Not sure yet..

Depending on io::IoStreams could be avoided by making the printing functions return a WString
That would require more allocations and more verbose handling at the call sites, so it's not ideal.

@danielrainer if we add a IoStreams method as above, it shouldn't affect call sites.
We could also avoid needless allocations if we hide IoStreams behind a trait.

impl Error {
    fn finish(w: impl std::io::Write);
}

but I guess there's no need to do that yet.

+    pub fn finish(self, streams: &mut IoStreams) {
+        self.append_to(streams.err);
+    }

I wonder if we should enforce that all errors are used,
i.e. someone calls finish() or something like let _ = err.to_string().
Probably there's too many exceptions and too little test coverage.

+                    err_str!("Cannot specify multiple positions")
+                        .with_cmd(CMD)

given how often these builder methods are used, maybe we should drop the with_ prefix?

+                        err_fmt!(Error::COMBO2_EXCLUSIVE, flag1, flag2)
+                            .with_cmd(&opts.name)
+                            .finish(streams);
+                        // TODO: STATUS_INVALID_ARGS
                        return Err(STATUS_CMD_ERROR);

yeah this should return STATUS_INVALID_ARGS

err_fmt!(Error::UNKNOWN_OPT,

I wonder if we should introduce a BuiltinError namespace or similar, for errors
that are only meaningful to builtins. Not sure, but that'd be close to the status quo.

I'm hesitating to add with_option(wstr)

I guess that can be decided later as well?
At least COMBO2_EXCLUSIVE is already straightforward to use.

The main concern here is either a combinatorial explosion in print_msg (for a total of 9
branches), or the deep-ish nesting of wgettext_fmt! calls (cmd/subcmd > options > msg >
who knows how many to construct msg)

if it's used a lot that might be okay.
But we can probably also find ways to defeat the combinatorial explosion,
maybe we can use local macros to compute the format strings...

+        match (self.cmd, self.subcmd) {
+            (None, _) => {
+                output.appendln(&self.msg);
+            }
+            (Some(cmd), None) => {
+                output.appendln(&wgettext_fmt!("%s: %s", cmd, &self.msg));
+            }
+            (Some(cmd), Some(subcmd)) => {
+                output.appendln(&wgettext_fmt!("%s: %s: %s", cmd, subcmd, &self.msg));
+            }
+        }

could extract the output.appendln call here, by introducing a local variable.
At least if we add more cases.

err.append_assign_to_msg('\n');
err.finish(streams);
err.append_to_msg('\n').finish(streams);

I wonder if we can improve the naming of the append methods.
A Rust naming convention for methods that consume self is "to_*" or "take".
Maybe "append" and "appending"..

"Fish does not have shell options. See `help %s`.",

I think we usually use lowercase fish

@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Mar 25, 2026

I wouldn't mind squashing most commits, no strong opinion.

I split it for easier code review, especially the localization part (ensuring the messages are moved or copied, instead of replaced with an empty translation).

Initially I was also thinking it would be useful in case we need to revert specific builtins but 1) there is likely too much interdependence on the localization (a revert is likely going to cause tons of merge conflicts), 2) the PR has changed from the idea of homogenizing the error messages to just an improvement of the internal API, i.e. less user-facing changes, so I don't see a need to "revert" instead of "fix" anymore.

So if the current commits have been reviewed, I'll squashed most of them for the final version.

-# CHECKERR: string repeat: Invalid count value '-1'
+# CHECKERR: string: repeat: Invalid count value '-1'

I like the old formatting (without the colon) better, it seems more natural.

Easy enough to change now :)
I used the double colon because that seemed to be the preferred format.

I think streams.append(err_fmt!(...)) would be a natural invocation, because that's what we already do for strings.

It wouldn't be streams.append but streams.err.append (more typing and even less discoverable). The streams name is also used everywhere for the var name as well as for the finish parameter, so .f<tab> is enough for the IDE to fill in the rest, including the final ;. It was really convenient while writing this code.

Maybe we should rename "finish" to "write_to" or similar. Not sure yet..

write_to is already used (to write to a generic OutputStream). For that matter finish is just a call to write_to(streams.err).

+    pub fn finish(self, streams: &mut IoStreams) {
+        self.append_to(streams.err);
+    }

I wonder if we should enforce that all errors are used, i.e. someone calls finish() or something like let _ = err.to_string().

Easy enough, at least currently since the with_xxx function are consuming.
(I'm hesitating to use a &mut self instead, which would allow the same function to be used to self-assign (err.with_cmd(...); err.with_subcmd(...);) or to chain (err.with_cmd(...).with_subcmd(...);). Currently, for "self-assigning", we need to do err = err.with_cmd(...);, which is uglier. Anyways, in that case, #[must_use] wouldn't work)

Probably there's too many exceptions and too little test coverage.

What do you mean by exceptions?
As for test coverage, because there is too little is why a must_use would be important. Otherwise the test would find that we forgot to do something with the error. That said, I'm working on improving the coverage (with limitations)

given how often these builder methods are used, maybe we should drop the with_ prefix?

I was thinking of APIs like Vec::with_capacity, but I think that's mostly for constructors, not builder patterns. Will change.

yeah this should return STATUS_INVALID_ARGS

I wasn't sure about changing the user-facing API (well, more so than what I'm already doing, although changing a status code is more risky than changing a translatable error message), or even if that should be done in this PR. Thus the TODO.

err_fmt!(Error::UNKNOWN_OPT,

I wonder if we should introduce a BuiltinError namespace or similar, for errors that are only meaningful to builtins. Not sure, but that'd be close to the status quo.

Currently, my Error is just used with builtins. I didn't realize it would be the case when I started on it (although the location of shared.rs should have been a big hint 😛). with_cmd implies a strong relationship as well.
So maybe the whole class should be move to the builtins directory instead of being in src, that way the builtin work will be in the name path.

I'm hesitating to add with_option(wstr)

I guess that can be decided later as well?

Indeed, which is why I didn't push the question.

At least COMBO2_EXCLUSIVE is already straightforward to use.

The main point for a with_option would be to enforce a standard error format, same as cmd/subcmd. Currently, sometime the option is followed by a colon, sometimes not. Sometimes the option is preceded by a colon, sometimes not (but then this PR is fixing that one indirectly since it's just a rephrasing of cmd-followed-by-a-colon-or-not)

The main concern here is either a combinatorial explosion in print_msg (for a total of 9
branches), or the deep-ish nesting of wgettext_fmt! calls (cmd/subcmd > options > msg >
who knows how many to construct msg)

if it's used a lot that might be okay. But we can probably also find ways to defeat the combinatorial explosion, maybe we can use local macros to compute the format strings...

I'm not sure that macro would work, not a macro_rule at least. Macro_proc should but that's an even bigger problem.

could extract the output.appendln call here, by introducing a local variable. At least if we add more cases.

ok

err.append_assign_to_msg('\n');
err.finish(streams);
err.append_to_msg('\n').finish(streams);

I wonder if we can improve the naming of the append methods. A Rust naming convention for methods that consume self is "to_*" or "take". Maybe "append" and "appending"..

to_xxx is consuming because it's a conversion/cast function. append_to_xxx is different, both in name and function.
append is not good IMHO because it's too generic (what am I appending? a msg? a cmd? ...). Similarly append_msg could mean "append a msg to the error", aka "set the msg". rather than "append to the existing msg", and it could also mean "postfix the error with some extra msg", i.e. "cmd: msg \n trailer \n extra_msg".

"Fish does not have shell options. See `help %s`.",

I think we usually use lowercase fish

I noticed, but this PR is just changing the error infrastructure. My goal is to later followup with a more user-facing PR to homogenized the error messages (there are a several error message that mean the same thing but say/present it differently, especially regarding options and combo of options).

@danielrainer
Copy link
Copy Markdown

danielrainer commented Mar 25, 2026

if we add a IoStreams method as above, it shouldn't affect call sites.

I'm assuming you're referring to streams.err.append(err_fmt!(...)). That seems like a good way of handling it. We'd only have to implement IntoCharIter for Error.

I don't think passing an impl std::io::Write argument is a good idea, since that trait provides fn write(&mut self, buf: &[u8]) -> Result<usize>;. To to proper PUA-decoding, we'd be converting from char's to a byte slice, which is not free for our widestrings, and then in the implementation of write we'd have to parse that to chars again and perform the PUA-decoding. Parsing would be cheap if we use std::str::from_utf8_unchecked, but I don't like the approach overall; it seems unnecessarily brittle and complicated.

@krobelus
Copy link
Copy Markdown
Contributor

krobelus commented Mar 26, 2026 via email

@Nahor Nahor marked this pull request as ready for review April 4, 2026 22:42
@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Apr 4, 2026

The PR is now ready to fully review, and can be merged once #12603 is

@krobelus krobelus added this to the fish 4.7 milestone Apr 5, 2026
krobelus pushed a commit that referenced this pull request Apr 5, 2026
To homogenize error reporting format, use a new Error struct. Currently this
is used for builtins and ensuring a common cmd/subcmd prefix.

Part of #12556
krobelus pushed a commit that referenced this pull request Apr 5, 2026
@Nahor Nahor mentioned this pull request Apr 5, 2026
@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Apr 5, 2026

@krobelus any particular reason the last two commits haven't been merged?

Added a commit that should fix the spurious Cirrus error in tmux-set.fish

@Nahor Nahor force-pushed the error_rewrite branch 2 times, most recently from 5e0f425 to 0f6cbe1 Compare April 5, 2026 20:54
@krobelus
Copy link
Copy Markdown
Contributor

krobelus commented Apr 7, 2026

@krobelus any particular reason the last two commits haven't been merged?

no, I was just busy

src/builtins/error.rs

this filename is weirdly ambiguous because one might think it represents an "error" builtin.
I guess this is not a new issue because we already have a cursed "shared.rs" (in addition to "mod.rs"),
but it would be nice if we could come up with an obvious solution (especially in case we ever add an "error" builtin).
Maybe this should be "src/builtins/shared/error.rs"?
AFAIK could still use the same import paths by re-exporting everything in shared,
i.e. have src/builtins/mod.rs do something like pub use crate::builtins::shared::*;.

err_str!(Error::TOO_MANY_ARGUMENTS)

Good. I like the conciseness of "Error", and the "builtin" part is still visible in the "use" statement.

macro_rules! err_fmt
macro_rules! err_str

the branching between string literal / expr (LocalizableString) is not needed, we can simply pass on any expression, wgettext! will still branch on it AFAICT:

diff --git a/src/builtins/error.rs b/src/builtins/error.rs
index 37a55f86ab..04848e7ad5 100644
--- a/src/builtins/error.rs
+++ b/src/builtins/error.rs
@@ -8,48 +8,33 @@
 };

 use fish_widestring::wstr;

 #[macro_export]
 macro_rules! err_fmt {
     (
-        $string:literal // format string
-        $(, $args:expr)* // list of expressions
-        $(,)?   // optional trailing comma
-    ) => {
-        $crate::builtins::error::Error::new(
-            $crate::wgettext_fmt!($string, $($args),*).into()
-        )
-    };
-    (
-        $string:expr // format string (LocalizableString)
+        $string:expr // format string (literal or LocalizableString)
         $(, $args:expr)* // list of expressions
         $(,)?   // optional trailing comma
     ) => {
         $crate::builtins::error::Error::new(
             $crate::wgettext_fmt!($string, $($args),*).into()
         )
     };
 }
 pub use err_fmt;

 #[macro_export]
 macro_rules! err_str {
     (
-        $string:literal // format string
+        $string:expr // format string (literal or LocalizableString)
         $(,)?   // optional trailing comma
     ) => {
         $crate::builtins::error::Error::new(std::borrow::Cow::Borrowed($crate::wgettext!($string)))
     };
-    (
-        $string:expr // format string (LocalizableString)
-        $(,)?   // optional trailing comma
-    ) => {
-        $crate::builtins::error::Error::new(std::borrow::Cow::Borrowed($crate::wgettext!(&$string)))
-    };
 }
 pub use err_str;

 /// Generate an `Error` from a string without localization. This is typically
 /// for error messages from external sources (e.g. C `strerror()`)
 #[macro_export]
 macro_rules! err_raw {

macro_rules! err_fmt
macro_rules! err_str

I wonder if err_str should be renamed to err.
Then this would be consistent with wgettext{,_fmt}.
I guess that's not a blocker.
I wonder if err! is used in similar ways in error handling libraries.

err_raw

I was wondering if this is the best name.
The use of "raw" in this context reminds me of std::io::Error::from_raw_os_error,
where "raw" implies legacy C error code as opposed to Rust enum.

What we mean here is either "not-localized" or "already localized".

I don't have a better suggestion, so err_raw is fine with me.

-                    panic!("argc should not be zero"); // should have been caught by the above test
+                    unreachable!("argc should not be zero"); // should have been caught by the above test

Yeah; panic and unreachable communicate almost the same thing but I tend to agree that unreachable is better for assumptions that are true (barring a compiler bug etc.).
panic might be more appropriate for assumptions that might be broken by changes in external crates and libc etc.. not too sure

    fn from(_: crate::wutil::wcstoi::Error) -> Self {
+    fn from(error: crate::builtins::error::Error<'a>) -> Self {

maybe we should use crate::{wutil, builtins} to shorten these names.

-                self.fields = arg.unwrap().try_into().map_err(|e| match e {
-                    FieldParseError::Number => StringError::NotANumber,
+                let arg = arg.unwrap();
+                self.fields = arg.try_into().map_err(|e| match e {
+                    FieldParseError::Number => err_fmt!(Error::NOT_NUMBER, arg),

Good change.
It introduces a bit of inconsistency because now some "invalid integer" errors use NotANumber while others use InvalidArgs(Error<'a>) but that's understandable.
In future, we should probably get rid of NotANumber completely because it doesn't add context (i.e. the argument that failed to parse).

error rewrite: remove unused error from shared

When looking at src/ only, it would make more sense to squash this commit.
I guess the split is to keep translation changes easier to review.
I wonder if that helped; I haven't reviewed them yet.

-                streams.err.appendln(&wgettext_fmt!(
-                    BUILTIN_ERR_ARG_COUNT2,
-                    cmd,
-                    s.to_wstr(),
-                    0,
-                    args.len()
-                ));
+                err_fmt!(error::Error::ARG_COUNT1, 0, args.len())
+                    .subcmd(cmd, s.to_wstr())
+                    .finish(streams);

the BUILTIN_ERR_ARG_COUNT{0,1,2} naming was weird, I don't really know why it exists, maybe just for lack of better naming.

How about making the names reflect the format string better (can be done later of course):

ARG_COUNT0 -> ARG_MISSING
ARG_COUNT1 -> ARGS_EXPECTED_GOT
ARG_COUNT2 -> ARGS_EXPECTED_GOT_WITH_CONTEXT
-    fn fatal_error<Str: AsRef<wstr>>(&mut self, errstr: Str) {
-        let errstr = errstr.as_ref();

this code merge could be an independent commit, right?

append_assign_to_msg

maybe this should be called borrowed_append_to_msg.
At least in C++, assign operations may still return a reference to self/*this, so I thought "borrowed" is closer to what we are actually doing.
The opposite of "borrow" would be "take" but that's the implied default for this struct.

+                err_str!("builtin history delete --exact requires --case-sensitive")
+                    .finish(streams);

In future, we could extract the "builtin history" and "delete" into cmd/subcmd calls.
(As other changes in this PR, this reduces context in translation sources, but AIUI, with fluent we can add arbitrary context manually.)

diff --git a/localization/po/zh_TW.po b/localization/po/zh_TW.po> -msgid "%s %s: unrecognized feature '%s'"
-msgstr "%s %s:不認識的功能「%s」"
+msgid "unrecognized feature '%s'"
+msgstr ""

I'm not sure if we should drop translations on the floor (and leave it to translators) just because we remove %s %s:/%s %s: prefixes.
Could we maybe write a program that automatically adjusts translations.
Python has a polib package that might help.
An added bonus is that it will be easier to review -- sometimes reviewing the program is a reasonable substitute for checking every single change.

Something like this pseudo code:

old = polib_parse("git show origin/master:translation/po/de.po")
new = polib_parse("cat translation/po/de.po")
dropped_messages = old.msgids - messages.msgids
for msgid in dropped_messages:
    for prefix in ("%s: ", "%s: %s: ", ...):
        if not msgid.startswith(prefix):
            continue
        assert old[msgid].startswith(prefix)
        messages[new] = old[msgid].removeprefix(prefix)
        break
save(new)
+msgid "%s %s: %s"
+msgstr ""

we should translate this one for all languages (using Chinese-style colons where appropriate).
AFAICT, that's the only "new" translation, all others should be modifications.. though I haven't checked the translation files.

krobelus pushed a commit that referenced this pull request Apr 7, 2026
@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Apr 8, 2026

Maybe this should be "src/builtins/shared/error.rs"?

Done

the branching between string literal / expr (LocalizableString) is not needed

Done

I wonder if err_str should be renamed to err.

I like the uniformity of err_str compared to the other err_xxx. It also make searching for it easier. "err" is too generic, and overused (variables, fields, functions, ...).

The use of "raw" in this context reminds me of std::io::Error::from_raw_os_error,
where "raw" implies legacy C error code as opposed to Rust enum.

"raw" just means "unprocessed"/"as-is". For legacy C, that means "unconverted to Rust errors". Rust "raw string" (r#) means "no escaping"). And here it means "no translation".

maybe we should use crate::{wutil, builtins} to shorten these names

Not needed after other changes I just made 😊

In future, we should probably get rid of NotANumber completely because it doesn't add context (i.e. the argument that failed to parse).

Done. UnknownOption could also be removed (and thus the whole StringError) but currently the subcmds don't have access to their name (Transform in particular has two names). Easily enough to fix but this need to touch more code, so punted to later.

error rewrite: remove unused error from shared

When looking at src/ only, it would make more sense to squash this commit.

Squashed

the BUILTIN_ERR_ARG_COUNT{0,1,2} naming was weird

Done. Went with MISSING_ARG, UNEXP_ARG_COUNT, UNPEXP_ARG_COUNT_WITH_CTX to be a bit more consistent with the other constants.
Also fixed a handful of others while at it ({MIN,MAX}_ARG_COUNT1, COMBOXXX)

this code merge could be an independent commit, right?

I thought it was too minor to warrant its own commit that's all, like the "panic!" vs "unreachable!" earlier.

maybe this should be called borrowed_append_to_msg.

I used append_assign to copy the {add,sub,div,...}{,_assign} from Rust.
To me "borrow" is more about "temporarily taking" an inner value out of its container, which is not the case here. "borrow" would fine if it was for something like err.borrow_mut_msg() += some_str.

I'm not sure if we should drop translations on the floor

I had a script that tries to preserve the translations. However, it worked by removing the prefix if it's the same between the reference and translated string, which is not the case for Chinese. Script attached below for the record.

AFAICT, that's the only "new" translation, all others should be modifications

There is at least one new translation: "fish does not have shell options [...]" in builtins/set.rs.

translation update script
import polib
import copy

lang = ["de", "en", "es", "fr", "ja_JP", "pl", "pt_BR", "sv", "zh_CN", "zh_TW"]
prefixes = ["%s: ", "%s: %s: ", "%s %s: "]
zh_separator = ":"

for lang in lang:
    pofile = polib.pofile(f"localization/po/{lang}.po.old")

    # Add entries without prefix
    for entry in pofile.translated_entries():
        for prefix in prefixes:
            if not entry.msgid.startswith(prefix):
                continue
            newid = entry.msgid.removeprefix(prefix)
            if newid == "" or newid in prefixes:
                continue
            existing_entry = pofile.find(newid)

            msgstr_prefix = prefix.replace(": ", zh_separator) if lang.startswith("zh") else prefix
            if entry.msgstr.startswith(msgstr_prefix):
                newstr = entry.msgstr.removeprefix(msgstr_prefix)
                if existing_entry is None:
                    newentry = copy.copy(entry)
                    newentry.msgid = newid
                    newentry.msgstr = newstr
                    pofile.append(newentry)
                elif existing_entry.translated():
                    if existing_entry.msgstr != newstr:
                        print(f"=== Bad match ({lang})\n- '{existing_entry.msgstr}'\n+ '{newstr}'")
                    pass
                elif newid == newstr:
                    pass
                else:
                    existing_entry.msgstr = newstr

    # Add entries for removed prefixes
    if lang.startswith("zh"):
        for prefix in prefixes:
            newid = f"{prefix}%s"
            newstr = newid.replace(": ", zh_separator)
            existing_entry = pofile.find(newid)
            if existing_entry is None:
                    newentry = polib.POEntry(
                        msgid = newid,
                        msgstr = newstr
                    )
                    pofile.append(newentry)
            elif existing_entry.translated():
                if existing_entry.msgstr != newstr:
                    print(f"=== Bad match ({lang})\n- '{existing_entry.msgstr}'\n+ '{newstr}'")
                pass
            else:
                existing_entry.msgstr = newstr

    pofile.save(f"localization/po/{lang}.po")

@danielrainer
Copy link
Copy Markdown

As other changes in this PR, this reduces context in translation sources, but AIUI, with fluent we can add arbitrary context manually.

Depends on what you mean by that. Fluent would have names for all variables, which of course adds context, but it's the same context for all users of that message.

I've also had a look at the translations and it seems that the translations with the same prefix as the English messages were already updated. This leaves the Chinese messages, which seem to be handled in the script you just posted, but also French messages, which use a non-breaking space before the colon. (Except for #12617.) I could post code for updating the French translations, but it's probably more convenient for you to integrate the changes yourself. In case it helps, here's a diff to be applied upon "error rewrite: use new Error to report errors", after rebasing that on #12617:

diff --git a/localization/po/fr.po b/localization/po/fr.po
index 66c1391225..b0ac8631f0 100644
--- a/localization/po/fr.po
+++ b/localization/po/fr.po
@@ -242,11 +242,11 @@
 
 #, c-format
 msgid "%s option requires %s"
-msgstr ""
+msgstr "l’option %s doit être utilisée avec %s"
 
 #, c-format
 msgid "%s requires a non-empty string"
-msgstr ""
+msgstr "%s doit être utilisée avec une chaîne non-vide"
 
 #, c-format
 msgid "%s variable"
@@ -342,11 +342,11 @@
 
 #, c-format
 msgid "%s: cannot use reserved keyword as function name"
-msgstr ""
+msgstr "%s : impossible d’utiliser un mot-clé réservé comme nom de fonction"
 
 #, c-format
 msgid "%s: contains a syntax error"
-msgstr ""
+msgstr "%s : contient une erreur de syntaxe"
 
 #, c-format
 msgid "%s: expected %d arguments; got %d"
@@ -370,7 +370,7 @@
 
 #, c-format
 msgid "%s: invalid base value"
-msgstr ""
+msgstr "%s : valeur de base invalide"
 
 #, c-format
 msgid "%s: invalid conversion specification"
@@ -378,7 +378,7 @@
 
 #, c-format
 msgid "%s: invalid function name"
-msgstr ""
+msgstr "%s : nom de fonction invalide"
 
 #, c-format
 msgid "%s: invalid integer"
@@ -386,7 +386,7 @@
 
 #, c-format
 msgid "%s: invalid mode"
-msgstr ""
+msgstr "%s : mode invalide"
 
 #, c-format
 msgid "%s: invalid mode name. See `help %s`"
@@ -406,7 +406,7 @@
 
 #, c-format
 msgid "%s: invalid scale"
-msgstr ""
+msgstr "%s : échelle invalide"
 
 #, c-format
 msgid "%s: invalid subcommand"
@@ -414,7 +414,7 @@
 
 #, c-format
 msgid "%s: invalid variable name. See `help %s`"
-msgstr ""
+msgstr "%s : nom de variable invalide. Voir « help %s »"
 
 #, c-format
 msgid "%s: missing argument"
@@ -450,7 +450,7 @@
 
 #, c-format
 msgid "%s: unexpected positional argument"
-msgstr ""
+msgstr "%s : argument positionnel inattendu"
 
 #, c-format
 msgid "%s: unknown option"
@@ -462,23 +462,23 @@
 
 #, c-format
 msgid "'%s' is a broken symbolic link to '%s'"
-msgstr ""
+msgstr "« %s » est un lien symbolique cassé vers « %s »"
 
 #, c-format
 msgid "'%s' is not a directory"
-msgstr ""
+msgstr "« %s » n’est pas un dossier"
 
 #, c-format
 msgid "'%s' is not a job"
-msgstr ""
+msgstr "« %s » n’est pas une tâche"
 
 #, c-format
 msgid "'%s' is not a valid job ID"
-msgstr ""
+msgstr "« %s » n’est pas un ID de tâche valide"
 
 #, c-format
 msgid "'%s' is not a valid process ID"
-msgstr ""
+msgstr "« %s » n’est pas un ID de processus valide"
 
 msgid "'break' while not inside of loop"
 msgstr "« break » hors d’une boucle"
@@ -532,7 +532,7 @@
 msgstr ""
 
 msgid "--command cannot be combined with --position=command"
-msgstr ""
+msgstr "--command ne peut pas être utilisée avec l’option --position=command"
 
 msgid "--end and --length are mutually exclusive"
 msgstr ""
@@ -550,7 +550,7 @@
 msgstr ""
 
 msgid "--set-cursor argument cannot be empty"
-msgstr ""
+msgstr "l’option --set-cursor ne peut pas être vide"
 
 msgid "--tokens options are mutually exclusive"
 msgstr ""
@@ -564,11 +564,11 @@
 
 #, c-format
 msgid "Abbreviation %s already exists, cannot rename %s"
-msgstr ""
+msgstr "l’abréviation %s existe déjà ; impossible de renommer %s"
 
 #, c-format
 msgid "Abbreviation '%s' cannot have spaces in the word"
-msgstr ""
+msgstr "l’abréviation « %s » ne peut pas avoir d’espaces dans son nom"
 
 msgid "Abbreviation expansion"
 msgstr ""
@@ -594,17 +594,17 @@
 msgstr "Toujours"
 
 msgid "Ambiguous job"
-msgstr ""
+msgstr "Tâche ambiguë"
 
 msgid "An error occurred while setting up pipe"
 msgstr "Une erreur est survenue lors du paramétrage du tube"
 
 msgid "An option spec must have at least a short or a long flag"
-msgstr ""
+msgstr "Une spécification d’option doit avoir au moins un nom court ou long"
 
 #, c-format
 msgid "Argument '%s' is out of range"
-msgstr ""
+msgstr "L’argument « %s » est hors limites"
 
 #, c-format
 msgid "Argument is not a number: '%s'"
@@ -644,18 +644,18 @@
 msgstr ""
 
 msgid "Can not specify scope when removing block"
-msgstr ""
+msgstr "Ne peut pas indiquer la portée en enlevant le bloc"
 
 msgid "Can not use the no-execute mode when running an interactive session"
 msgstr "Le mode no-execute ne peut pas être utilisé dans une session interactive"
 
 #, c-format
 msgid "Can't put job %d, '%s' to foreground because it is not under job control"
-msgstr ""
+msgstr "Impossible de mettre la tâche %d, « %s » au premier plan, car elle n’est pas gérée par le contrôleur de tâches"
 
 #, c-format
 msgid "Can't put job %s, '%s' to background because it is not under job control"
-msgstr ""
+msgstr "Impossible de mettre la tâche %s, « %s » en arrière plan, car elle n’est pas gérée par le contrôleur de tâches"
 
 #, c-format
 msgid "Cannot add control modifier to control character '%s'"
@@ -663,19 +663,19 @@
 
 #, c-format
 msgid "Cannot combine options %s"
-msgstr ""
+msgstr "Impossible de combiner les options %s"
 
 msgid "Cannot specify multiple positions"
-msgstr ""
+msgstr "Impossible de spécifier plusieurs positions"
 
 msgid "Cannot specify multiple regex patterns"
-msgstr ""
+msgstr "Impossible de spécifier plusieurs expressions régulières"
 
 msgid "Cannot specify multiple set-cursor options"
-msgstr ""
+msgstr "Impossible de spécifier plusieurs options set-cursor"
 
 msgid "Cannot use --append or --prepend when assigning to a slice"
-msgstr ""
+msgstr "Impossible d’utiliser --append ou --prepend lors de l’assignation à une tranche"
 
 msgid "Cannot use stdin (fd 0) as pipe output"
 msgstr "Impossible d’utiliser l’entrée standard (fd 0) comme sortie de tube"
@@ -708,7 +708,7 @@
 msgstr ""
 
 msgid "Command not valid at an interactive prompt"
-msgstr ""
+msgstr "Commande invalide dans une invite interactive"
 
 msgid "Commandname was invalid"
 msgstr ""
@@ -724,18 +724,18 @@
 
 #, c-format
 msgid "Could not find '%s'"
-msgstr ""
+msgstr "Impossible de trouver « %s »"
 
 #, c-format
 msgid "Could not find a job with process ID '%d'"
-msgstr ""
+msgstr "Impossible de trouver une tâche avec l’ID de processus « %d »"
 
 #, c-format
 msgid "Could not find child processes with the name '%s'"
-msgstr ""
+msgstr "Impossible de trouver des processus enfants avec le nom « %s »"
 
 msgid "Could not find home directory"
-msgstr ""
+msgstr "Dossier personnel introuvable"
 
 #, c-format
 msgid "Could not find job '%d'"
@@ -775,7 +775,7 @@
 
 #, c-format
 msgid "Did you mean `set %s %s`?"
-msgstr ""
+msgstr "Vouliez-vous dire « set %s %s » ?"
 
 msgid "Division by zero"
 msgstr ""
@@ -788,14 +788,14 @@
 
 #, c-format
 msgid "Empty directory '%s' does not exist"
-msgstr ""
+msgstr "Le dossier vide « %s » n’existe pas"
 
 msgid "End a block of commands"
 msgstr "Termine un bloc de commandes"
 
 #, c-format
 msgid "Error encountered while sourcing file '%s':"
-msgstr ""
+msgstr "Erreur lors de l’inclusion du fichier « %s »"
 
 #, c-format
 msgid "Error reading script file '%s':"
@@ -811,7 +811,7 @@
 
 #, c-format
 msgid "Error while reading file '%s'"
-msgstr ""
+msgstr "Erreur lors de la lecture du fichier « %s »"
 
 #, c-format
 msgid "Error: %s"
@@ -849,7 +849,7 @@
 
 #, c-format
 msgid "Expected %s for %s"
-msgstr ""
+msgstr "Seul %s est attendu comme valeur pour %s"
 
 #, c-format
 msgid "Expected %s, but found %s"
@@ -870,13 +870,13 @@
 msgstr "Un nom de variable est attendu après un $."
 
 msgid "Expected at least one argument"
-msgstr ""
+msgstr "Au moins un argument est attendu"
 
 msgid "Expected exactly one function name"
-msgstr ""
+msgstr "Exactement un nom de fonction est attendu"
 
 msgid "Expected exactly two names (current function name, and new function name)"
-msgstr ""
+msgstr "Exactement deux noms sont attendus (ancien et nouveau nom de fonction)"
 
 msgid "Expected file path to read/write for -w:"
 msgstr ""
@@ -921,11 +921,11 @@
 
 #, c-format
 msgid "Function '%s' already exists. Cannot create copy of '%s'"
-msgstr ""
+msgstr "La fonction « %s » existe déjà. Impossible de créer la copie de « %s »"
 
 #, c-format
 msgid "Function '%s' does not exist"
-msgstr ""
+msgstr "La fonction « %s » n’existe pas"
 
 msgid "Generate random number"
 msgstr "Génère un nombre aléatoire"
@@ -974,18 +974,18 @@
 
 #, c-format
 msgid "Illegal function name '%s'"
-msgstr ""
+msgstr "« %s » est un nom de fonction incorrect"
 
 msgid "Illegal instruction"
 msgstr "Instruction illégale"
 
 #, c-format
 msgid "Implicit int flag '%c' already defined"
-msgstr ""
+msgstr "Le sémaphore implicitement entier « %c » est déjà défini"
 
 #, c-format
 msgid "Implicit int short flag '%c' does not allow modifiers like '%c'"
-msgstr ""
+msgstr "Le sémaphore court implicitement entier « %c » n’autorise pas de modificateurs comme « %c »"
 
 #, c-format
 msgid "Incomplete escape sequence '%s'"
@@ -1006,15 +1006,15 @@
 
 #, c-format
 msgid "Invalid --max-args value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de --max-args est invalide"
 
 #, c-format
 msgid "Invalid --min-args value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de --min-args est invalide"
 
 #, c-format
 msgid "Invalid --unknown-arguments value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de --unknown-arguments est invalide"
 
 #, c-format
 msgid "Invalid arg: %s"
@@ -1025,11 +1025,11 @@
 
 #, c-format
 msgid "Invalid count value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de « count » est invalide"
 
 #, c-format
 msgid "Invalid end value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de « end » est invalide"
 
 #, c-format
 msgid "Invalid escape sequence in pattern \"%s\""
@@ -1037,19 +1037,19 @@
 
 #, c-format
 msgid "Invalid escape style '%s'"
-msgstr ""
+msgstr "Le style d’échappement « %s » est invalide"
 
 #, c-format
 msgid "Invalid fields value '%s'"
-msgstr ""
+msgstr "La valeur « %s » de « fields » est invalide"
 
 #, c-format
 msgid "Invalid function name: %s"
-msgstr ""
+msgstr "Nom de fonction invalide : %s"
 
 #, c-format
 msgid "Invalid index starting at '%s'"
-msgstr ""
+msgstr "Indice invalide à partir de « %s »"
 
 msgid "Invalid index value"
 msgstr ""
@@ -1059,19 +1059,19 @@
 
 #, c-format
 msgid "Invalid job control mode '%s'"
-msgstr ""
+msgstr "Mode de contrôle de tâche « %s » invalide"
 
 #, c-format
 msgid "Invalid length value '%s'"
-msgstr ""
+msgstr "La valeur de taille « %s » est invalide"
 
 #, c-format
 msgid "Invalid level value '%s'"
-msgstr ""
+msgstr "La valeur de niveau « %s » est invalide"
 
 #, c-format
 msgid "Invalid limit '%s'"
-msgstr ""
+msgstr "La limite « %s » est invalide"
 
 #, c-format
 msgid "Invalid max matches value '%s'"
@@ -1087,7 +1087,7 @@
 
 #, c-format
 msgid "Invalid option spec '%s' at char '%c'"
-msgstr ""
+msgstr "Option invalide « %s » au caractère « %c »"
 
 #, c-format
 msgid "Invalid padding character of width zero '%s'"
@@ -1119,7 +1119,7 @@
 
 #, c-format
 msgid "Invalid start value '%s'"
-msgstr ""
+msgstr "Indice de départ « %s » invalide"
 
 #, c-format
 msgid "Invalid style value '%s'"
@@ -1127,7 +1127,7 @@
 
 #, c-format
 msgid "Invalid token '%s'"
-msgstr ""
+msgstr "Lexème invalide « %s »"
 
 #, c-format
 msgid "Invalid type '%s'"
@@ -1162,7 +1162,7 @@
 msgstr ""
 
 msgid "Key not specified"
-msgstr ""
+msgstr "Clé non spécifiée"
 
 msgid "Language specifiers appear repeatedly:"
 msgstr ""
@@ -1196,7 +1196,7 @@
 msgstr ""
 
 msgid "Missing -- separator"
-msgstr ""
+msgstr "Séparateur -- manquant"
 
 msgid "Missing closing parenthesis"
 msgstr ""
@@ -1216,7 +1216,7 @@
 msgstr ""
 
 msgid "Name cannot be empty"
-msgstr ""
+msgstr "le nom ne peut pas être vide"
 
 msgid "Negate exit status of job"
 msgstr "Inverser le code de retour de la tâche"
@@ -1229,18 +1229,18 @@
 
 #, c-format
 msgid "No abbreviation named %s with the specified command restrictions"
-msgstr ""
+msgstr "aucune abréviation nommée %s"
 
 #, c-format
 msgid "No binding found for key '%s'"
-msgstr ""
+msgstr "Aucun lien trouvé pour la combinaison %s"
 
 #, c-format
 msgid "No binding found for key sequence '%s'"
 msgstr ""
 
 msgid "No blocks defined"
-msgstr ""
+msgstr "Aucun bloc défini"
 
 msgid "No catalogs available for language specifiers:"
 msgstr ""
@@ -1251,7 +1251,7 @@
 
 #, c-format
 msgid "No suitable job: %d"
-msgstr ""
+msgstr "Aucune tâche appropriée : %d"
 
 #, c-format
 msgid "No suitable job: %s"
@@ -1312,7 +1312,7 @@
 
 #, c-format
 msgid "Permission denied: '%s'"
-msgstr ""
+msgstr "Permission refusée « %s »"
 
 #, c-format
 msgid "Please set $%s to a directory where you have write access."
@@ -1387,7 +1387,7 @@
 
 #, c-format
 msgid "Regular expression substitute error: %s"
-msgstr ""
+msgstr "Erreur de substitution de l’expression régulière : %s"
 
 msgid "Remove job from job list"
 msgstr "Supprimer la tâche de la liste des tâches"
@@ -1400,10 +1400,10 @@
 msgstr "Redirection demandée à « %s », qui n’est pas un descripteur de fichier valide"
 
 msgid "Requires at least two arguments"
-msgstr ""
+msgstr "doit être utilisée avec au moins deux arguments"
 
 msgid "Requires exactly two arguments"
-msgstr ""
+msgstr "doit être utilisée avec exactement deux arguments"
 
 msgid "Resource limit not available on this operating system"
 msgstr ""
@@ -1471,7 +1471,7 @@
 
 #, c-format
 msgid "Short flag '%c' invalid, must be alphanum or '#'"
-msgstr ""
+msgstr "Le sémaphore « %c » est invalide : il doit être alphanumérique ou valoir « # »"
 
 msgid "Show absolute path sans symlinks"
 msgstr ""
@@ -1551,7 +1551,7 @@
 
 #, c-format
 msgid "The directory '%s' does not exist"
-msgstr ""
+msgstr "Le dossier « %s » n’existe pas"
 
 #, c-format
 msgid "The error was '%s'."
@@ -1615,7 +1615,7 @@
 
 #, c-format
 msgid "Tried to change the read-only variable '%s'"
-msgstr ""
+msgstr "Impossible de modifier la variable en lecture seule « %s »"
 
 #, c-format
 msgid "Tried to modify the special variable '%s' to an invalid value"
@@ -1668,7 +1668,7 @@
 
 #, c-format
 msgid "Unexpected argument -- '%s'"
-msgstr ""
+msgstr "argument inattendu -- « %s »"
 
 msgid "Unexpected end of string, expecting ')'"
 msgstr ""
@@ -1697,7 +1697,7 @@
 
 #, c-format
 msgid "Unknown color '%s'"
-msgstr ""
+msgstr "Couleur  « %s » inconnue"
 
 msgid "Unknown command"
 msgstr ""
@@ -1719,7 +1719,7 @@
 
 #, c-format
 msgid "Unknown error trying to locate directory '%s'"
-msgstr ""
+msgstr "Erreur inconnue en tentant de localiser le dossier « %s »"
 
 msgid "Unknown error while evaluating command substitution"
 msgstr ""
@@ -1733,11 +1733,11 @@
 
 #, c-format
 msgid "Unknown input function '%s'"
-msgstr ""
+msgstr "Fonction d’entrée « %s » inconnue"
 
 #, c-format
 msgid "Unknown signal '%s'"
-msgstr ""
+msgstr "Signal « %s » inconnu"
 
 msgid "Unmatched wildcard"
 msgstr ""
@@ -1796,7 +1796,7 @@
 msgstr ""
 
 msgid "`set --show` does not allow slices with the var names"
-msgstr ""
+msgstr "« set --show » n’autorise pas de tranches avec les noms des variables"
 
 msgid "a path variable"
 msgstr "une variable de chemin"
@@ -1874,11 +1874,11 @@
 
 #, c-format
 msgid "exclusive flag '%s' is not valid"
-msgstr ""
+msgstr "le sémaphore exclusif « %s » est invalide"
 
 #, c-format
 msgid "exclusive flag string '%s' is not valid"
-msgstr ""
+msgstr "le sémaphore texte exclusif « %s » est invalide"
 
 #, c-format
 msgid "expected %d arguments; got %d"
@@ -1958,7 +1958,7 @@
 
 #, c-format
 msgid "job %d ('%s') was stopped and has been signalled to continue."
-msgstr ""
+msgstr "la tâche %d (« %s ») a été arrêtée et a reçu un signal pour continuer."
 
 msgid "line/column index starts at 1"
 msgstr ""
@@ -2016,7 +2016,7 @@
 msgstr "arrêtée"
 
 msgid "subcommand takes no options"
-msgstr ""
+msgstr "cette sous-command n’a pas d’option"
 
 #, c-format
 msgid "successfully set universal '%s'; but a global by that name shadows it"
@@ -2054,7 +2054,7 @@
 
 #, c-format
 msgid "unrecognized feature '%s'"
-msgstr ""
+msgstr "fonctionnalité non reconnue « %s »"
 
 #, c-format
 msgid "variable '%s' is read-only"

@danielrainer
Copy link
Copy Markdown

And for "error rewrite: remove unused error from shared":

diff --git a/localization/po/fr.po b/localization/po/fr.po
index 14851fb19e..69b4e61a82 100644
--- a/localization/po/fr.po
+++ b/localization/po/fr.po
@@ -194,7 +194,7 @@
 
 #, c-format
 msgid "%s %s: options cannot be used together"
-msgstr ""
+msgstr "%s %s : ces options ne peuvent pas être utilisées ensembles"
 
 #, c-format
 msgid "%s (line %d)"
@@ -310,7 +310,7 @@
 
 #, c-format
 msgid "%s: invalid integer"
-msgstr ""
+msgstr "%s : entier invalide"
 
 #, c-format
 msgid "%s: invalid mode"
@@ -318,7 +318,7 @@
 
 #, c-format
 msgid "%s: invalid mode name. See `help %s`"
-msgstr ""
+msgstr "%s : nom de mode invalide. Voir « help %s »"
 
 #, c-format
 msgid "%s: invalid option argument: %s"
@@ -330,7 +330,7 @@
 
 #, c-format
 msgid "%s: invalid subcommand"
-msgstr ""
+msgstr "%s : sous-commande invalide"
 
 #, c-format
 msgid "%s: invalid variable name. See `help %s`"
@@ -342,11 +342,11 @@
 
 #, c-format
 msgid "%s: option does not take an argument"
-msgstr ""
+msgstr "%s : cette option ne prend pas d’argument"
 
 #, c-format
 msgid "%s: option requires an argument"
-msgstr ""
+msgstr "%s : cette option doit être utilisée avec un argument"
 
 #, c-format
 msgid "%s: unexpected positional argument"
@@ -354,7 +354,7 @@
 
 #, c-format
 msgid "%s: unknown option"
-msgstr ""
+msgstr "%s : option inconnue"
 
 #, c-format
 msgid "%s: value not completely converted (can't convert '%s')"
@@ -639,7 +639,7 @@
 
 #, c-format
 msgid "Could not find job '%d'"
-msgstr ""
+msgstr "Impossible de trouver la tâche « %d »"
 
 msgid "Could not set terminal mode for new job"
 msgstr "Impossible de paramétrer le mode du terminal pour la nouvelle tâche"
@@ -979,7 +979,7 @@
 
 #, c-format
 msgid "Invalid max value '%s'"
-msgstr ""
+msgstr "Valeur maximale « %s » invalide"
 
 #, c-format
 msgid "Invalid number: %s"
@@ -1283,7 +1283,7 @@
 
 #, c-format
 msgid "Regular expression compile error: %s"
-msgstr ""
+msgstr "Erreur de compilation de l’expression régulière: %s"
 
 #, c-format
 msgid "Regular expression substitute error: %s"
@@ -1471,7 +1471,7 @@
 msgstr ""
 
 msgid "There are no suitable jobs"
-msgstr ""
+msgstr "Aucune tâche appropriée"
 
 msgid "There are still jobs active:"
 msgstr "Des tâches sont toujours actives :"
@@ -1782,7 +1782,7 @@
 
 #, c-format
 msgid "expected %d arguments; got %d"
-msgstr ""
+msgstr "%d arguments attendus ; %d reçus"
 
 #, c-format
 msgid "expected <= %d arguments; got %d"
@@ -1843,7 +1843,7 @@
 msgstr "précision invalide : %s"
 
 msgid "invalid subcommand"
-msgstr ""
+msgstr "sous-commande invalide"
 
 #, c-format
 msgid "invalid underline style: %s"

@Nahor
Copy link
Copy Markdown
Contributor Author

Nahor commented Apr 8, 2026

Updated script for french:

Details
import polib
import copy

lang = ["de", "en", "es", "fr", "ja_JP", "pl", "pt_BR", "sv", "zh_CN", "zh_TW"]
prefixes = ["%s: ", "%s: %s: ", "%s %s: "]
zh_separator = ":"
fr_separator = " : "

for lang in lang:
    pofile = polib.pofile(f"localization/po/{lang}.po.old")

    # Add entries without prefix
    for entry in pofile.translated_entries():
        for prefix in prefixes:
            if not entry.msgid.startswith(prefix):
                continue
            newid = entry.msgid.removeprefix(prefix)
            if newid == "" or newid in prefixes:
                continue
            existing_entry = pofile.find(newid)

            if lang.startswith("zh"):
                msgstr_prefix = prefix.replace(": ", zh_separator)
            elif lang.startswith("fr"):
                msgstr_prefix = prefix.replace(": ", fr_separator)
            else:
                msgstr_prefix = prefix
            if entry.msgstr.startswith(msgstr_prefix):
                newstr = entry.msgstr.removeprefix(msgstr_prefix)
                if existing_entry is None:
                    newentry = copy.copy(entry)
                    newentry.msgid = newid
                    newentry.msgstr = newstr
                    pofile.append(newentry)
                elif existing_entry.translated():
                    if existing_entry.msgstr != newstr:
                        print(f"=== Bad match ({lang})\n- '{existing_entry.msgstr}'\n+ '{newstr}'")
                    pass
                elif newid == newstr:
                    pass
                else:
                    existing_entry.msgstr = newstr

    # Add entries for removed prefixes
    if lang.startswith("zh"):
        for prefix in prefixes:
            newid = f"{prefix}%s"
            newstr = newid.replace(": ", zh_separator)
            existing_entry = pofile.find(newid)
            if existing_entry is None:
                    newentry = polib.POEntry(
                        msgid = newid,
                        msgstr = newstr
                    )
                    pofile.append(newentry)
            elif existing_entry.translated():
                if existing_entry.msgstr != newstr:
                    print(f"=== Bad match ({lang})\n- '{existing_entry.msgstr}'\n+ '{newstr}'")
                pass
            else:
                existing_entry.msgstr = newstr
    if lang.startswith("fr"):
        for prefix in prefixes:
            newid = f"{prefix}%s"
            newstr = newid.replace(": ", fr_separator)
            existing_entry = pofile.find(newid)
            if existing_entry is None:
                    newentry = polib.POEntry(
                        msgid = newid,
                        msgstr = newstr
                    )
                    pofile.append(newentry)
            elif existing_entry.translated():
                if existing_entry.msgstr != newstr:
                    print(f"=== Bad match ({lang})\n- '{existing_entry.msgstr}'\n+ '{newstr}'")
                pass
            else:
                existing_entry.msgstr = newstr

    pofile.save(f"localization/po/{lang}.po")

To homogenize error reporting format, use a new Error struct. Currently this
is used for builtins and ensuring a common cmd/subcmd prefix.
Use the more generic `StringError::InvalidArgs` instead
@krobelus
Copy link
Copy Markdown
Contributor

krobelus commented Apr 8, 2026

this code merge could be an independent commit, right?

I thought it was too minor to warrant its own commit that's all, like the "panic!" vs "unreachable!" earlier.

panic!/unreachable! is immediately obvious (given that jj show highlights the word that was changed) but this change spans multiple lines and my diff tool is not smart enough to tell me that there was basically no functional change.


There was a missing French translation for the Invalid value for '--color' option message
(which was one of the messages that was missing the non-breaking space, so maybe this is due to a bad merge? Anyway, I fixed it).
Other than that, translations seem good now. I didn't write my own diff script to check it but the script seems correct.

krobelus pushed a commit that referenced this pull request Apr 8, 2026
krobelus pushed a commit that referenced this pull request Apr 8, 2026
To homogenize error reporting format, use a new Error struct. Currently this
is used for builtins and ensuring a common cmd/subcmd prefix.

Part of #12556
@krobelus krobelus closed this in d649c2a Apr 8, 2026
@Nahor Nahor deleted the error_rewrite branch April 8, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants