Externalize Fluent localization; Windows test context refactor + audit#260
Merged
Externalize Fluent localization; Windows test context refactor + audit#260
Conversation
…o Fluent Add a detailed ExecPlan document (3-7-1-externalize-user-facing-strings.md) outlining the roadmap for migrating all user-facing strings to Fluent localization bundles. The plan covers scope, constraints, risks, progress steps, and validation criteria for implementing Fluent key audits, Spanish translations, localization infrastructure, testing, and documentation updates for roadmap item 3.7.1. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdd a localisation subsystem and export Fluent keys; wire Arc‑shared localisers into CLI and runtime; replace many hard‑coded messages with LocalizedMessage; attach localized messages to multiple error/diagnostic variants; add a build‑time Fluent key auditor; and update tests and test‑support to use localisation fixtures and expectations. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(120,150,200,0.5)
participant Build as Build Script
participant Keys as Keys Parser
participant FTL_en as FTL Parser (en‑US)
participant FTL_es as FTL Parser (es‑ES)
participant Auditor as Auditor
participant Cargo as Cargo
end
Build->>Keys: Read src/localization/keys.rs
Keys->>Keys: Extract define_keys! body and parse string literals
Keys-->>Build: Declared keys set
Build->>FTL_en: Read locales/en‑US/messages.ftl
FTL_en->>FTL_en: Parse message identifiers
FTL_en-->>Build: en‑US keys set
Build->>FTL_es: Read locales/es‑ES/messages.ftl
FTL_es->>FTL_es: Parse message identifiers
FTL_es-->>Build: es‑ES keys set
Build->>Auditor: Compare declared vs bundles
Auditor->>Auditor: Compute missing / orphaned keys
alt Issues found
Auditor->>Cargo: Emit formatted error and fail build
else
Auditor->>Cargo: Continue build
end
sequenceDiagram
rect rgba(180,200,150,0.5)
participant Main as main.rs
participant CLI as CLI parser
participant Localizer as Global Localizer
participant Config as Config Merge
participant RuntimeLocalizer as Runtime Localizer Builder
end
Main->>Localizer: Set initial Arc<Localizer>
Main->>CLI: Configure validation parsers with Arc localizer
CLI->>Localizer: Request validation messages during parse
Localizer-->>CLI: Localized values / error messages
CLI-->>Main: Parsed CLI and config
Main->>Config: Merge configs and determine runtime locale
Main->>RuntimeLocalizer: build_localizer(runtime_locale)
RuntimeLocalizer-->>Main: Arc localiser for runtime
Main->>Localizer: set_localizer(runtime Arc)
Main->>CLI: Reconfigure validation parsers with runtime Arc
AnyError->>Localizer: Render LocalizedMessage via Display
Localizer-->>AnyError: Formatted text for display
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideImplements a centralized Fluent-based localization framework (with English fallback and Spanish translations) across CLI, runtime, stdlib, and diagnostics, adds a build-time key audit to enforce key coverage, and updates tests and docs to assert localized behavior and mark roadmap item 3.7.1 as complete. Sequence diagram for CLI locale selection and localised error renderingsequenceDiagram
actor User
participant Shell
participant Main as main_rs
participant CliLoc as cli_localization
participant LocMod as localization_module
participant Cli as cli_rs
participant Runner as runner_mod
User->>Shell: run `netsuke --locale es-ES build`
Shell->>Main: start process with args
Main->>CliLoc: build_localizer(locale_hint_or_env)
CliLoc-->>Main: FluentLocalizer(en_US+es_ES)
Main->>LocMod: set_localizer(Arc<FluentLocalizer>)
Main->>Cli: set_validation_localizer(Arc<FluentLocalizer>)
Main->>Cli: parse_with_localizer_from(args, localizer)
Cli-->>Main: Cli struct, ArgMatches
Main->>Runner: generate_ninja(&Cli)
Runner->>LocMod: message(RUNNER_MANIFEST_NOT_FOUND)
LocMod->>LocMod: build LocalizedMessage with args(manifest_name,directory)
LocMod-->>Runner: formatted Spanish string
Runner-->>Main: Err(RunnerError::ManifestNotFound { message, help })
Main->>LocMod: localizer() for rendering Diagnostic
Main-->>Shell: print Spanish error and hint to stderr
Class diagram for localisation core types and error integrationclassDiagram
class Localizer~trait~ {
<<external>>
+message(key: &str, args: Option~LocalizationArgs~, fallback: &str) String
}
class LocalizationArgs {
<<external>>
}
class localization_module {
<<module>>
+set_localizer(localizer: Arc~Localizer~) void
+localizer() Arc~Localizer~
+set_localizer_for_tests(localizer: Arc~Localizer~) LocalizerGuard
+message(key: &'static str) LocalizedMessage
}
class LocalizerGuard {
-previous: Arc~Localizer~
+drop() void
}
class LocalizedMessage {
+key: &'static str
+args: Vec~(&'static str, String)~
+new(key: &'static str) LocalizedMessage
+with_arg(name: &'static str, value: ToString) LocalizedMessage
+to_string() String
}
class localization_keys {
<<module>>
+CLI_USAGE: &str
+RUNNER_MANIFEST_NOT_FOUND: &str
+MANIFEST_PARSE: &str
+IR_RULE_NOT_FOUND: &str
+NINJA_GEN_MISSING_ACTION: &str
+HOST_PATTERN_EMPTY: &str
+NETWORK_POLICY_SCHEME_EMPTY: &str
+STDLIB_FETCH_URL_INVALID: &str
+STDLIB_PATH_IO_NOT_FOUND: &str
+STDLIB_WHICH_NOT_FOUND: &str
+ALL_KEYS: &'static [&'static str]
}
class RunnerError {
<<enum>>
ManifestNotFound
-manifest_name: String
-directory: String
-path: PathBuf
-message: LocalizedMessage
-help: LocalizedMessage
}
class ManifestError {
<<enum>>
Parse
-source: Box~Diagnostic~
-message: LocalizedMessage
}
class IrGenError {
<<enum>>
+RuleNotFound
+MultipleRules
+EmptyRule
+DuplicateOutput
+CircularDependency
+ActionSerialisation
+InvalidCommand
-message: LocalizedMessage
}
class NinjaGenError {
<<enum>>
+MissingAction
+Format
-message: LocalizedMessage
}
class NetworkPolicyConfigError {
<<enum>>
+EmptyScheme
+InvalidScheme
+EmptyAllowlist
-message: LocalizedMessage
}
class NetworkPolicyViolation {
<<enum>>
+SchemeNotAllowed
+MissingHost
+HostNotAllowlisted
+HostBlocked
-message: LocalizedMessage
}
class HostPatternError {
<<enum>>
+Empty
+ContainsScheme
+ContainsSlash
+MissingSuffix
+EmptyLabel
+InvalidCharacters
+InvalidLabelEdge
+LabelTooLong
+HostTooLong
-message: LocalizedMessage
}
localization_module --> LocalizedMessage : builds
localization_module --> Localizer : uses
localization_module --> localization_keys : keys_*
LocalizerGuard --> localization_module : restores
LocalizedMessage --> LocalizationArgs : builds args
RunnerError --> LocalizedMessage : message,help
ManifestError --> LocalizedMessage : message
IrGenError --> LocalizedMessage : message
NinjaGenError --> LocalizedMessage : message
NetworkPolicyConfigError --> LocalizedMessage : message
NetworkPolicyViolation --> LocalizedMessage : message
HostPatternError --> LocalizedMessage : message
class cli_module {
<<module>>
+set_validation_localizer(localizer: Arc~Localizer~) void
-validation_message(key: &str, args: Option~LocalizationArgs~, fallback: &str) String
}
cli_module --> Localizer : uses
cli_module --> localization_keys : uses
Flow diagram for build-time Fluent key auditflowchart TD
A_Start[Start build.rs] --> B_ReadKeys
B_ReadKeys[extract_key_constants
from src/localization/keys.rs] --> C_ReadEn
C_ReadEn[extract_ftl_keys
from locales/en-US/messages.ftl] --> D_ReadEs
D_ReadEs[extract_ftl_keys
from locales/es-ES/messages.ftl] --> E_Compare
E_Compare[Compute differences
between declared and en-US/es-ES keys] --> F_CheckMissing
F_CheckMissing{Any missing
keys?} -->|No| G_Success[Audit passes
continue build]
F_CheckMissing -->|Yes| H_Error[Emit error message
with missing keys per locale
and fail build]
H_Error --> I_End[Build script returns Err]
G_Success --> I_End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
…ndles - Extract all user-facing strings into Fluent `.ftl` catalogs for English and Spanish. - Introduce `src/localization/keys.rs` to centralize Fluent message keys as source of truth. - Implement a compile-time audit in `build.rs` that validates presence of Fluent keys across catalogs, failing CI on missing keys. - Update numerous modules (`cli`, `runner`, `manifest`, `stdlib`, etc.) to use localized messages instead of hardcoded strings. - Add support in CLI for localized validation messages and help texts with snake_case keys. - Integrate localization with error types to display translated diagnostics messages and hints. - Introduce helper types and APIs in `src/localization/mod.rs` for global localizer management and rendering. - Add Spanish translations alongside English, used as reference language. This change enables full localization support, ensures message key consistency, and improves user-facing error and help output internationalization. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file let mut keys = BTreeSet::new();
for line in source.lines() {
let trimmed = line.trim_start();
if trimmed.is_empty() || trimmed.starts_with('#') || trimmed.starts_with('.') {❌ New issue: Complex Conditional |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +74 to +80 if let Some(about_key) = subcommand_about_key(&name) {
if let Some(about) = updated.get_about().map(ToString::to_string) {
let message = localizer.message(about_key, None, &about);
updated = updated.about(message);
} else if let Some(message) = localizer.lookup(about_key, None) {
updated = updated.about(message);
}❌ New issue: Bumpy Road Ahead |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +73 to +79 .map_err(|err| {
io_to_error(
path,
&localization::message(keys::STDLIB_PATH_ACTION_STAT),
err,
)
})❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…dir helper - Introduced with_parent_dir helper to encapsulate opening parent directory and error mapping. - Refactored file_size, read_utf8, and open_file functions to use the new helper. - In cli_l10n, extracted localize_field function to reduce duplication when localizing command fields. - In build.rs, extracted should_skip_ftl_line function to clean up conditionals in extract_ftl_keys. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
…or clarity Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
Changed the type of the action_key parameter in with_parent_dir function from &str to &'static str for improved clarity and potential lifetime benefits. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
…positives Add module-level suppressions of clippy and unused_assignments lints due to false positives from miette/thiserror macros that vary across Rust versions. This workaround prevents lint failures until upstream fixes are available. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
tests/std_filter_tests/command_filters.rs (1)
1-12: Add a module-level doc comment and decompose this file.This file lacks the required
//!doc comment explaining its purpose. Additionally, at 729 lines, it exceeds the 400-line limit.Split the tests into separate submodules by concern (e.g.,
shell_filter_tests.rs,grep_filter_tests.rs,windows_filter_tests.rs) and add a module doc to each. As per coding guidelines, large modules must be decomposed.src/stdlib/config.rs (2)
427-463: Replace hard-coded limit errors with localization keys.Avoid brittle English literals by deriving expected messages from keys.
♻️ Proposed refactor
- #[case::output(CommandLimitCase { - builder: StdlibConfig::with_command_max_output_bytes, - accessor: |cfg: &StdlibConfig| cfg.command_max_output_bytes, - default_value: DEFAULT_COMMAND_MAX_OUTPUT_BYTES, - updated: 2_048, - zero_err: "Command output capture limit must be positive.", - })] + #[case::output(CommandLimitCase { + builder: StdlibConfig::with_command_max_output_bytes, + accessor: |cfg: &StdlibConfig| cfg.command_max_output_bytes, + default_value: DEFAULT_COMMAND_MAX_OUTPUT_BYTES, + updated: 2_048, + zero_err_key: keys::STDLIB_COMMAND_OUTPUT_LIMIT_POSITIVE, + })] @@ - #[case::stream(CommandLimitCase { - builder: StdlibConfig::with_command_max_stream_bytes, - accessor: |cfg: &StdlibConfig| cfg.command_max_stream_bytes, - default_value: DEFAULT_COMMAND_MAX_STREAM_BYTES, - updated: 65_536, - zero_err: "Command stream limit must be positive.", - })] + #[case::stream(CommandLimitCase { + builder: StdlibConfig::with_command_max_stream_bytes, + accessor: |cfg: &StdlibConfig| cfg.command_max_stream_bytes, + default_value: DEFAULT_COMMAND_MAX_STREAM_BYTES, + updated: 65_536, + zero_err_key: keys::STDLIB_COMMAND_STREAM_LIMIT_POSITIVE, + })] @@ - let err = (case.builder)(base_config, 0).expect_err("zero-byte limits must be rejected"); - assert_eq!(err.to_string(), case.zero_err); + let err = (case.builder)(base_config, 0).expect_err("zero-byte limits must be rejected"); + let expected = localization::message(case.zero_err_key).to_string(); + assert_eq!(err.to_string(), expected); @@ - zero_err: &'static str, + zero_err_key: &'static str,
492-509: Drive remaining test expectations from localization keys.Remove hard-coded strings in which-cache and skip-dir tests by building expected messages from keys.
♻️ Proposed refactor
- assert_eq!(err.to_string(), "Which cache capacity must be positive."); + let expected = + localization::message(keys::STDLIB_WHICH_CACHE_CAPACITY_POSITIVE).to_string(); + assert_eq!(err.to_string(), expected); @@ - #[case(vec![""], "Skip directory entries must not be empty.")] - #[case(vec!["."], "Skip directory entries must not contain '..'.")] - #[case(vec![".."], "Skip directory entries must not contain '..'.")] - #[case(vec!["dir/name"], "Skip directory entries must not contain path separators.")] - #[case(vec!["dir\\name"], "Skip directory entries must not contain path separators.")] + #[case(vec![""], keys::STDLIB_SKIP_DIR_EMPTY)] + #[case(vec!["."], keys::STDLIB_SKIP_DIR_NAVIGATION)] + #[case(vec![".."], keys::STDLIB_SKIP_DIR_NAVIGATION)] + #[case(vec!["dir/name"], keys::STDLIB_SKIP_DIR_SEPARATOR)] + #[case(vec!["dir\\name"], keys::STDLIB_SKIP_DIR_SEPARATOR)] fn workspace_skip_dirs_validate_inputs( base_config: StdlibConfig, #[case] entries: Vec<&str>, - #[case] message: &str, + #[case] message_key: &'static str, ) { let err = base_config .with_workspace_skip_dirs(entries) .expect_err("invalid skip entries should error"); - assert_eq!(err.to_string(), message); + let expected = localization::message(message_key).to_string(); + assert_eq!(err.to_string(), expected); }src/cli.rs (1)
1-4: File exceeds 400 line limit.At 437 lines, this file exceeds the 400 line guideline. Decompose by extracting the validation localizer infrastructure (lines 33-64) and/or the parsing functions (
parse_jobs,parse_scheme,parse_locale) into a separate module. As per coding guidelines, large modules must be decomposed.src/ir/graph.rs (7)
70-89: Update doc examples to include themessagefield.The doc example constructs
IrGenError::EmptyRulewithout the newly requiredmessagefield. This will causecargo test --docto fail.🐛 Proposed fix
/// fn describe(err: IrGenError) -> String { /// match err { -/// IrGenError::EmptyRule { target_name } => { +/// IrGenError::EmptyRule { target_name, .. } => { /// format!("{target_name} missing rule") /// }, /// other => other.to_string(), /// } /// } /// +/// use netsuke::localization; /// assert_eq!( /// describe(IrGenError::EmptyRule { /// target_name: "app".into(), +/// message: localization::message("ir-empty-rule"), /// }), /// "app missing rule" /// );
94-117: Update doc example to include themessagefield.The construction at lines 99-102 omits the required
messagefield. Add the field or use a helper to construct the error.🐛 Proposed fix
+/// use netsuke::localization; /// let err = IrGenError::RuleNotFound { /// target_name: "app".into(), /// rule_name: "compile".into(), +/// message: localization::message("ir-rule-not-found"), /// };
119-144: Update doc example to include themessagefield.The construction at lines 125-128 omits the required
messagefield.🐛 Proposed fix
+/// use netsuke::localization; /// let err = IrGenError::MultipleRules { /// target_name: "lib".into(), /// rules: vec!["c".into(), "cpp".into()], +/// message: localization::message("ir-multiple-rules"), /// };
146-164: Update doc example to include themessagefield and fix assertion.The construction omits the required
messagefield. Additionally, the assertion at line 154-156 expects a hardcoded string, but the display now uses the localized message.🐛 Proposed fix
+/// use netsuke::localization; -/// let err = IrGenError::EmptyRule { target_name: "docs".into() }; -/// assert_eq!( -/// err.to_string(), -/// "No rules specified for target docs" -/// ); +/// let msg = localization::message("ir-empty-rule").with_arg("target", "docs"); +/// let err = IrGenError::EmptyRule { +/// target_name: "docs".into(), +/// message: msg.clone(), +/// }; +/// assert_eq!(err.to_string(), msg.to_string());
166-188: Update doc example to include themessagefield.The construction at lines 172-174 omits the required
messagefield, and the pattern match at line 175 should use..to ignore additional fields.🐛 Proposed fix
+/// use netsuke::localization; /// let err = IrGenError::DuplicateOutput { /// outputs: vec!["obj.o".into()], +/// message: localization::message("ir-duplicate-outputs"), /// }; -/// if let IrGenError::DuplicateOutput { outputs } = err { +/// if let IrGenError::DuplicateOutput { outputs, .. } = err {
190-216: Update doc example to include themessagefield.The construction at lines 197-200 omits the required
messagefield.🐛 Proposed fix
+/// use netsuke::localization; /// let err = IrGenError::CircularDependency { /// cycle: vec![Utf8PathBuf::from("a"), Utf8PathBuf::from("a")], /// missing_dependencies: Vec::new(), +/// message: localization::message("ir-circular-dependency"), /// };
218-260: Update doc examples to match struct variant signatures.The
ActionSerialisationexample at line 225 uses tuple syntax but the variant is now a struct withsourceandmessagefields. TheInvalidCommandexample omits themessagefield and the assertion expects a hardcoded string.🐛 Proposed fix for ActionSerialisation
+/// use netsuke::localization; /// let source = serde_json::Error::custom("invalid action"); -/// let err = IrGenError::ActionSerialisation(source); +/// let err = IrGenError::ActionSerialisation { +/// source, +/// message: localization::message("ir-action-serialisation"), +/// }; /// assert!(err.to_string().contains("invalid action"));🐛 Proposed fix for InvalidCommand
+/// use netsuke::localization; +/// let msg = localization::message("ir-invalid-command") +/// .with_arg("snippet", "echo $in"); /// let err = IrGenError::InvalidCommand { /// command: "echo $in".into(), /// snippet: "echo $in".into(), +/// message: msg.clone(), /// }; -/// assert_eq!( -/// err.to_string(), -/// "command is not a valid shell command: echo $in" -/// ); +/// assert_eq!(err.to_string(), msg.to_string());src/manifest/diagnostics/mod.rs (1)
126-135: Update doc example to include themessagefield.The doc example constructs
ManifestError::Parsewithout the newly requiredmessagefield. This will causecargo test --docto fail.🐛 Proposed fix
+/// use netsuke::localization; -/// let err = ManifestError::Parse { source: Box::new(miette!("bad manifest")) }; -/// assert_eq!(format!("{err}"), "manifest parse error"); +/// let msg = localization::message("manifest-parse"); +/// let err = ManifestError::Parse { +/// source: Box::new(miette!("bad manifest")), +/// message: msg.clone(), +/// }; +/// assert_eq!(format!("{err}"), msg.to_string());
🤖 Fix all issues with AI agents
In `@build.rs`:
- Around line 104-125: Add a doc comment above the extract_ftl_keys function
that succinctly explains what FTL structures it expects and which constructs it
intentionally skips: mention it reads a Fluent .ftl file, ignores
comment/metadata lines (via should_skip_ftl_line), skips attribute lines and
entries without '=' or with empty IDs, treats lines starting with '-' as
terms/invalid keys, and returns a sorted set of message IDs or an error if none
found; reference the function name extract_ftl_keys and helper
should_skip_ftl_line in the comment so future readers understand the parsing
assumptions and edge cases.
- Around line 81-98: Add a brief explanatory comment above the
extract_key_constants function describing the expected keys.rs format: lines
must include the pattern => "key" (i.e., a map entry where the RHS is a quoted
key) because the parser uses trimmed.split_once("=> \"") and
rest.split_once('"') to extract keys; mention that other formats or additional
whitespace/escaping may break extraction and that the function returns an error
if no keys found.
In `@docs/execplans/3-7-1-externalize-user-facing-strings.md`:
- Around line 92-96: Update the document text to standardise spelling to
en-GB-oxendict forms by replacing all occurrences of "localisation" and
"localiser" (and their capitalised variants) with "localization" and "localizer"
respectively; ensure the decision line references ortho_config::Localizer and
the Fluent resources under "locales/" use the new spelling, and scan the
surrounding prose to update any related phrases so all mentions consistently use
"localization"/"localizer".
- Around line 108-110: Update the two outcome bullets so they correctly state
that English is the reference locale and Spanish is used only as an example
locale: change the first bullet to say "Fluent keys now cover CLI copy,
diagnostics, and stdlib errors with English as the reference translation and
Spanish as an example locale (with English fallback)," and update the second
bullet to explicitly note that "The compile-time Fluent key audit prevents
missing-key regressions in CI by validating against the English reference."
Target the two outcome lines that start with "Fluent keys now cover..." and "The
compile-time Fluent key audit..." in the document.
- Around line 13-15: Update the opening list in the document so it uses the
Oxford comma: change "help, status, warnings, errors" to "help, status,
warnings, and errors" and scan the rest of the file (including the sentence
starting "Netsuke should render all user-facing text") to ensure all
comma-separated lists consistently include the Oxford comma (e.g., before the
final conjunction).
In `@docs/roadmap.md`:
- Around line 231-233: The roadmap shows a contradictory status: the parent item
"3.7.1. Externalize user-facing strings into Fluent `.ftl` bundles." is checked
but its subtask "Implement compile-time audit that fails CI on missing message
keys." is unchecked; update docs to reflect shipped state by marking the subtask
complete if the compile-time audit has been delivered, or revert the parent
checkbox on "3.7.1." to unchecked if the audit hasn’t shipped, and ensure
adjacent item "3.7.2. Implement locale resolution." remains unchanged.
In `@docs/users-guide.md`:
- Line 543: Update the heading text "Configuration and Localisation" to use
Oxford English "-ization" spelling as "Configuration and Localization" and
replace any other occurrences of the word "Localisation" in this document with
"Localization" to conform to en-GB-oxendict; look for the exact heading string
"Configuration and Localisation" and the token "Localisation" to locate and
change them.
- Around line 559-563: Change the British spelling "localised" to Oxford English
"localized" in the CLI docs string that describes --locale <LOCALE> and
NETSUKE_LOCALE (e.g., the sentence "Use `--locale <LOCALE>` or `NETSUKE_LOCALE`
to select localised CLI copy and error messages."). Update any other occurrences
in the same paragraph (including "user-facing runtime diagnostics" sentence) so
all instances use "localized" / "localized" consistently.
In `@locales/en-US/messages.ftl`:
- Line 205: Remove the two leading spaces from the message value for the key
stdlib.command.closed_input_early so it reads "Input closed early while writing
to the command." (locate the string for stdlib.command.closed_input_early and
trim the leading whitespace; do not change surrounding lines or spacing for
other concatenated messages.)
In `@locales/es-ES/messages.ftl`:
- Around line 205-212: The translated message values for the following
keys—stdlib.command.closed_input_early, stdlib.command.broken_pipe,
stdlib.command.terminated_by_signal, stdlib.command.exited_with_status,
stdlib.command.output_limit_exceeded, stdlib.command.timeout,
stdlib.command.exit_status_suffix, and stdlib.command.signal_suffix—have
unintended leading spaces; edit the values to remove the leading whitespace at
the start of each string so user-facing output does not include a prefixed space
while preserving placeholders (e.g., { $location }, { $status }, { $mode }, {
$stream }, { $limit }, { $seconds }) and the intended spacing around
parentheses.
In `@src/cli.rs`:
- Line 56: Add a brief inline comment above the const assertion to explain that
the line "const _: fn(Arc<dyn Localizer>) = set_validation_localizer;" is a
compile-time signature check: it ensures the function set_validation_localizer
has the exact type fn(Arc<dyn Localizer>) and will fail to compile if the
signature changes; mention both the const trick and the expected signature
(Arc<dyn Localizer>) so future readers understand its intent.
In `@src/localization/mod.rs`:
- Around line 64-65: Add a one-line comment immediately above the two const
assertions explaining they are compile-time signature checks that assert the
functions set_localizer and set_localizer_for_tests have the expected types
(fn(Arc<dyn Localizer>) and fn(Arc<dyn Localizer>) -> LocalizerGuard
respectively), so future readers understand these lines are intentional
type-level tests rather than unused code. Reference the symbols set_localizer
and set_localizer_for_tests in the comment for clarity.
In `@src/manifest/diagnostics/mod.rs`:
- Around line 8-18: Keep the module-level suppression block as-is but preserve
and clarify the existing comment and FIXME: retain the rationale explaining the
clippy circularity and the version-dependent thiserror/miette behavior, keep the
FIXME marker, and include a note that once upstream is fixed the module-level
#![allow(clippy::allow_attributes, clippy::allow_attributes_without_reason,
unused_assignments)] should be removed and replaced by item-level expectations
(#[expect(..., reason = "...")]) on the two Error derives referenced at lines
136 and 151; do not narrow or remove the current module-level allows now because
scoping them to items would re-trigger the clippy lints.
In `@src/manifest/tests/macros.rs`:
- Around line 47-49: Replace the hard-coded message strings in the #[case]
attributes for the test cases and use the correct localization keys that include
the SIGNATURE_ infix: change the second element of the cases to
"MANIFEST_MACRO_SIGNATURE_MISSING_PARAMS" for the "(name)" case and to
"MANIFEST_MACRO_SIGNATURE_MISSING_IDENTIFIER" for the " " and "greet" (or
other identifier-missing) cases; locate and update the attributes in the test
macro cases (the #[case(...)] lines shown) so they reference those exact key
names instead of the incorrect keys.
In `@src/ninja_gen.rs`:
- Around line 20-35: Update the doc comments for the error variants
MissingAction and Format to use en-GB-oxendict spelling by replacing “Localised”
with “Localized” where it describes the message field; locate the MissingAction
and Format enum variants and their docs that mention LocalizedMessage and change
the comment text accordingly so it reads “Localized error message.”
In `@src/runner/error.rs`:
- Around line 37-41: Update the doc comments for the struct fields referencing
LocalizedMessage: change the word "Localised" to "Localized" in the docstrings
for the message and help fields (the fields named message and help, which use
the LocalizedMessage type and the #[help] attribute) so the comments follow the
en-GB-oxendict spelling.
In `@src/runner/process/file_io.rs`:
- Around line 85-91: The error construction inside the closure passed to
ok_or_else is unnecessarily verbose; replace the anyhow!("{}",
localization::message(keys::RUNNER_IO_NO_EXISTING_ANCESTOR).with_arg("path",
path.as_str())) pattern with a simpler form that converts the LocalizedMessage
to a String (e.g., anyhow!(localization::message(...).with_arg("path",
path.as_str()).to_string())) or use ok_or_else(||
localization::message(...).with_arg(...).to_string().into()) so the Display
implementation is used directly; update the closure referenced around
ok_or_else() where path and
localization::message(keys::RUNNER_IO_NO_EXISTING_ANCESTOR) are used.
- Around line 100-106: Replace the indirect formatting that wraps the localized
message in a format string when constructing the error for Utf8Path::from_path;
instead of anyhow!("{}",
localization::message(keys::RUNNER_IO_NON_UTF8_PATH).with_arg("path",
path.display().to_string())), call .to_string() directly on the localized
message (the result of localization::message(...).with_arg(...)) and pass that
string into anyhow! so the error construction for utf8_path is simplified and
consistent with other uses in this file.
In `@src/stdlib/command/error.rs`:
- Around line 95-103: In io_error (and the similar suffix-appending code around
lines 162–170), ensure you insert an explicit separator before appending
localized suffixes instead of relying on translation whitespace: after
constructing the base message in io_error (and the other function), append a
single space or punctuation (e.g., message.push(' ') or message.push_str(": "))
before calling message.push_str(&localization::message(...).to_string()); this
guarantees proper separation regardless of translation content and keeps
Error::new(ErrorKind::InvalidOperation, message) unchanged.
In `@src/stdlib/command/quote.rs`:
- Around line 15-23: The Display impl for QuoteError currently allocates by
calling to_string() and passing that into f.write_str; update the fmt::Display
for QuoteError (the fmt::Formatter handling in impl fmt::Display for QuoteError)
to use write!(f, "{}", localization::message(keys::COMMAND_QUOTE_LINE_BREAK))
instead of creating an intermediate String so the message is written directly to
the formatter and avoids the extra allocation.
In `@src/stdlib/network/mod.rs`:
- Around line 337-346: The io_error function duplicates logic from
io_helpers::io_to_error; refactor to reuse a single helper: either extend
io_helpers::io_to_error to accept the extra context (action_key: &str and path:
&Utf8Path or path: &str) and return the localized minijinja::Error, or replace
io_error with a small wrapper that calls io_helpers::io_to_error after
converting the parameters (e.g., map action_key to localization message and path
to &str). Update any callers in this module to use the consolidated helper and
remove the duplicated io_error implementation so the localization and io->Error
conversion logic lives only in io_helpers::io_to_error (or the new shared
helper).
In `@src/stdlib/network/policy/mod.rs`:
- Around line 41-59: The doc comments use British spelling "Localised" instead
of the project's required "-ize" spelling; update the doc comments for the enum
variants EmptyScheme, InvalidScheme (the field `message: LocalizedMessage`
comment), and EmptyAllowlist to replace "Localised error message." with
"Localized error message." and apply the same change to the other occurrences in
the file (around the other block at lines ~356-384) so all doc comments
consistently use "Localized".
In `@src/stdlib/path/fs_utils.rs`:
- Around line 44-45: Update the doc comment in src/stdlib/path/fs_utils.rs that
begins "Execute an operation on a file's parent directory handle, translating
I/O errors with the appropriate localized action message." to use en-GB
spelling: replace "localized" with "localised" in that comment (the comment
above the function that executes an operation on a file's parent directory
handle in fs_utils.rs).
In `@src/stdlib/path/io_helpers.rs`:
- Around line 10-29: In io_to_error, normalize the label before performing the
case-insensitive contains check: compute a lower-cased version of the label
(e.g., label_lower) from the already-built label String and use
detail.to_ascii_lowercase().contains(label_lower.as_str()) instead of comparing
to the original capitalised label; update the conditional that branches to
STDLIB_PATH_IO_FAILED_WITH_DETAIL to use this normalized label so capitalised
labels don't skip the intended branch and duplicate the label in the final
message (references: function io_to_error, variable label, variable detail, and
io_error_kind_label).
In `@tests/manifest_glob_tests.rs`:
- Around line 181-183: The test's substring assertions for glob error messages
are locale-sensitive; before the assertions referencing msg and using ensure!
(in tests/manifest_glob_tests.rs) set the test localizer to a fixed locale
(e.g., en-US) or replace the hardcoded substrings with calls to
localization::message(...) for the expected texts; specifically, initialize the
locale at the start of the test (or before each assertion block) so the
ensure!(msg.contains(...)) checks always compare against the English/localized
message retrieved via the localization API rather than environment-dependent
strings.
In `@tests/runner_tool_subcommands_tests.rs`:
- Line 8: Pin the test locale before constructing the expected localized strings
so assertions are deterministic: call the existing test localization helper to
set the locale to "en-US" (or equivalent) via the test helper provided in the
project (e.g., the helper that configures netsuke::localization) at the start of
the test in tests/runner_tool_subcommands_tests.rs, before you build the
expected variable that uses localization keys (netsuke::localization::{self,
keys}); repeat the same change for the other test blocks noted (around lines
113-118) so all localization-dependent assertions use the pinned locale.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +167 to +168 let _lock = localizer_test_lock().expect("localizer test lock poisoned");
let _guard = set_en_localizer();❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…lock in tests - Update localizer_test_lock calls to use error propagation instead of expects. - Introduce helper assert_macro_registration_fails to reduce code duplication in macro registration tests. - Change test fixtures and BDD step definitions to handle poisoned localizer test lock via Result. - Simplify localizer test lock function formatting. These changes enhance robustness by properly handling poisoned mutex errors during tests and reduce duplicative code in macro error assertions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/bdd/steps/stdlib/config.rs`:
- Around line 34-47: The configure_localisation function currently always calls
localizer_test_lock(), causing deadlock if the same thread already holds the
guard in world.localization_lock; change it to first check whether
world.localization_lock already contains a guard and reuse it: if
world.localization_lock has a value, do not call localizer_test_lock(), just
replace the localisation guard via cli_localization::build_localizer and
localization::set_localizer_for_tests and update world.localization_guard;
otherwise acquire the lock with localizer_test_lock() and then set both
world.localization_lock and world.localization_guard as before. Ensure you keep
the existing calls to world.localization_guard.take_value() and
world.localization_lock.take_value() before setting new values, and preserve the
function's Result return type and error propagation from localizer_test_lock().
In `@tests/manifest_glob_tests.rs`:
- Around line 33-44: The map_err call in en_localizer (wrapping
localizer_test_lock()) discards the original poison error; replace the
map_err(|e| anyhow!(...)) chain with anyhow::Context by calling
.context("localizer test lock poisoned") (ensure anyhow::Context is in scope) so
the returned Result preserves the original error as a source; update the
en_localizer function to use localizer_test_lock().context("localizer test lock
poisoned") and keep the rest (cli_localization::build_localizer,
localization::set_localizer_for_tests, EnLocalizerFixture) unchanged.
In `@tests/runner_tool_subcommands_tests.rs`:
- Around line 36-38: The current use of map_err(anyhow::anyhow!("localizer test
lock poisoned: {e}")) on the Result from localizer_test_lock() discards the
original poison error chain; change that call to use anyhow::Context (e.g.,
localizer_test_lock().context("localizer test lock poisoned")?) so the original
error is preserved as the source, and apply the same change to the other
occurrence around set_en_localizer() at the later block (lines ~103-105); ensure
anyhow::Context is in scope so .context(...) can be used on the Result.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +190 to +191 fn glob_unmatched_bracket_errors(en_localizer: Result<EnLocalizerFixture>) -> Result<()> {
let (_lock, _guard) = en_localizer?;❌ New issue: Code Duplication |
1 similar comment
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +190 to +191 fn glob_unmatched_bracket_errors(en_localizer: Result<EnLocalizerFixture>) -> Result<()> {
let (_lock, _guard) = en_localizer?;❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Expand the comment documenting the module-level `#![allow]` suppression to better explain: - Why `#[expect]` cannot be used (version-dependent lint behaviour) - Why item-level suppression isn't feasible (clippy circular dependency) - Which Error derives will need item-level expectations once upstream miette/thiserror fixes are released 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add `en_localizer` rstest fixture in `src/manifest/tests/macros.rs` to eliminate duplicated localizer setup across test functions - Fix potential deadlock in `configure_localisation` BDD step by reusing existing lock when present instead of re-acquiring - Improve error chaining in `tests/manifest_glob_tests.rs` and `tests/runner_tool_subcommands_tests.rs` using `.context()` - Add `assert_glob_error_contains` helper function to reduce structural duplication in glob error tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/manifest/tests/macros.rs`:
- Around line 33-44: Change the en_localizer fixture to return a Result instead
of panicking: update the function signature of en_localizer to return
AnyResult<EnLocalizerFixture>, remove the .expect() call on
localizer_test_lock() and use the ? operator to propagate PoisonError (i.e., let
lock = localizer_test_lock()?;), then return Ok((lock, guard)). Update all
consumers of en_localizer to propagate errors (use ? where they currently assume
non-Result) so lock poisoning is handled rather than panicking; references:
EnLocalizerFixture type, en_localizer function, and localizer_test_lock().
In `@tests/bdd/steps/stdlib/config.rs`:
- Around line 43-53: Replace the separate is_some() check and take_value() call
with a single take to atomically acquire the optional lock: call
world.localization_lock.take_value() and fall back to
Some(localizer_test_lock()?) if it returns None, then later restore the taken
value with world.localization_lock.set_value(...) if present; locate this change
around the use of world.localization_lock, localizer_test_lock(), and the
subsequent
localization::set_localizer_for_tests/cli_localization::build_localizer calls
and update the logic accordingly to remove the is_some() branch and the
conditional restoration pattern.
…t fixtures - Change `en_localizer` fixture in `src/manifest/tests/macros.rs` to return `AnyResult` instead of using `.expect()`, propagating lock poisoning errors properly - Consolidate `register_manifest_macros_rejects_non_string_values` and `register_manifest_macros_requires_body` into a parameterized test `register_manifest_macros_invalid_macro_entry` to reduce code duplication - Simplify lock acquisition in `configure_localisation` BDD step by combining the check-and-take into a single `take_value().map_or_else()` operation, eliminating the TOCTOU gap 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/manifest/tests/macros.rs`:
- Around line 195-196: The assertion
`ensure!(err.to_string().contains(&expected), "{err}");` drops the expected
value from the failure message; update the failure message in this test to
include both the expected string (`expected`) and the actual error (`err`) so
failures show expected vs actual (follow the pattern used in
`parse_macro_name_errors`), i.e., change the `ensure!` invocation that
references `expected` and `err` to format both values into the message.
In `@tests/bdd/steps/stdlib/config.rs`:
- Around line 39-47: The current comment inaccurately describes the lock-taking
behaviour—update the comment above the let lock =
world.localization_lock.take_value().map_or_else(|| localizer_test_lock(), Ok)?
to say that it takes and returns an existing lock if present, otherwise calls
localizer_test_lock() to create/acquire a new lock (i.e., reuse existing lock
when Some, call localizer_test_lock() when None), and remove the reference to
"Using take_value().or_else()" so the wording matches the actual map_or_else
call.
…ments
- Use `anyhow::Error::msg()` instead of `anyhow!("{e}")` for cleaner
error conversion while preserving context chaining
- Improve assertion diagnostics in `assert_macro_registration_fails`
to show expected value alongside actual error, matching the pattern
used in `parse_macro_name_errors`
- Fix misleading comment that referenced `or_else` when code uses
`map_or_else`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ob tests
Use `anyhow::Error::msg(e.to_string())` instead of `anyhow!("{e}")` for
cleaner error conversion while preserving context chaining, matching the
pattern used in src/manifest/tests/macros.rs.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
📎 Task references preserved in existing plan and task notes.
Changes
Goals and outcomes
Plan of work (Stage A–D)
Validation and acceptance criteria
Interfaces and dependencies
Revision note
References