Promote try_parse_localized to a generic blanket trait (11.1.2)#350
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
|
| Layer / File(s) | Summary |
|---|---|
Core parsing API, Localizer extension, and module exports ortho_config/src/localizer/clap_command/parse.rs, ortho_config/src/localizer/clap_command/mod.rs, ortho_config/src/localizer/mod.rs, ortho_config/src/localizer/clap_error.rs, ortho_config/src/lib.rs |
Introduces parse_localized_command free function and LocalizedParse blanket trait in a new parse submodule; adds Localizer::locale() method with FluentLocalizer implementation; emits tracing::warn! on translation fallback with identifier/error_kind/locale fields; re-exports all new items through the module hierarchy. |
Integration tests, test doubles, and compile-time validation ortho_config/tests/localized_parse.rs, ortho_config/tests/localized_parse_trybuild.rs, ortho_config/tests/trybuild/localized_parse_*.rs, ortho_config/tests/trybuild/localized_parse_requires_parser.stderr, ortho_config/Cargo.toml |
Defines fixture CLI structures with nested subcommands and invalid Fluent identifier variant; creates test Localizer doubles (RecordingLocalizer, MissingSubcommandLocalizer, TranslatedLocalizer) and tracing event capture harness; validates successful parse with ArgMatches retention, NoOpLocalizer parity, error-context preservation, Fluent identifier coverage against message_id_for, panic on invalid identifier, and trait-bound enforcement via trybuild pass/fail fixtures. |
hello_world example parsing migration examples/hello_world/src/cli/localization.rs, examples/hello_world/src/cli/mod.rs, examples/hello_world/src/main.rs, examples/hello_world/src/cli/tests/localisation.rs, examples/hello_world/tests/localised_help.rs |
Removes ParsedCommandLine struct and three inherent parsing helpers; simplifies localization.rs to a single re-export; refactors parse_command_line() to construct pre-localised command and call parse_localized_command; migrates unit tests to the new function; adds assert_display_request_succeeds helper for --help/--version coverage across en-US and ja-JP locales. |
ADR-006 and CLI localisation design contract updates docs/adr-006-identifier-derivation-panics.md, docs/cli-localization-design.md |
Extends ADR-006 with LocalizedParse panic-surface observation. Substantially revises the CLI localisation design document covering promoted parsing surfaces, identifier normalisation and collision contracts, locale-resolution lifecycle and typestate observability, clap error coverage gates with build-time drift detection, eager error-context localisation, i18n-embed bridge semantics, derive behaviour and artefact emission, and updated failure modes. |
User and developer guidance, README, and CHANGELOG docs/users-guide.md, docs/developers-guide.md, docs/design.md, examples/hello_world/README.md, README.md, ortho_config/README.md, ortho_config_macros/README.md, CHANGELOG.md |
Documents LocalizedParse and parse_localized_command usage patterns with worked examples; adds parse-error enrichment rules; updates design references; refreshes the hello_world README; adds feature bullets and CHANGELOG entries for the new public items; clarifies JSON5 feature semantics. |
11.1.2 execution plan, roadmap completion, and living document updates docs/execplans/11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait.md, docs/roadmap.md, docs/execplans/*.md |
Introduces the 11.1.2 living execution plan with constraints, tolerances, risks, decision log D-1–D-5, validation criteria, and interface sketches; marks roadmap item 11.1.2 complete with implementation evidence and findings; reflows and updates all other execution plans for accuracy and consistency. |
Documentation-only line-reflow docs/adr-*.md, docs/*.md, docs/execplans/*.md, ortho_config_macros/README.md |
Markdown prose reflow across ADRs, guides, and reference documents without substantive content changes. |
Possibly Related Issues
- Update users-guide.md and create v0-9-0-migration-guide.md for CLI localisation surface (Phase 11) #346 – This PR directly implements the CLI localisation promotion that issue
#346requests, with extensive updates todocs/users-guide.mdanddocs/cli-localization-design.mdcoveringLocalizedParseandparse_localized_command. - Add CLI localisation surface section to developers-guide.md (Phase 11) #347 – This PR adds developer documentation to
docs/developers-guide.mdcovering the promoted CLI localisation surface, schema-ownership guidance, and parse-error enrichment patterns, directly addressing issue#347.
Possibly Related PRs
- leynos/ortho-config#238: This PR builds directly on PR
#238's FluentLocalizer/DemoLocalizerclap integration by refactoring example-specific parsing glue into the newparse_localized_command+LocalizedParseAPI and extending theLocalizertrait (addinglocale()). - leynos/ortho-config#243: This PR builds on the Fluent localisation integration from
#243by extendingortho_config'sLocalizertrait and adding clap-localised parsing APIs on top of that same localisation layer. - leynos/ortho-config#245: This PR extends the clap error Fluent-localisation infrastructure from PR
#245by adding warn-level tracing on translation fallback and strengthening error-context localisation semantics.
Suggested Reviewers
- codescene-delta-analysis
Poem
🦀 The parse glue is gone, no more bespoke mess,
A blanket trait covers eachParserno less,
parse_localized_commandstands proud and alone,
Fluent identifiers looked up, warnings are shown.
The roadmap ticks forward — 11.1.2 complete! 🎉
🚥 Pre-merge checks | ✅ 20
✅ Passed checks (20 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and directly summarises the primary changeset: promoting try_parse_localized to a blanket trait with roadmap reference (11.1.2). |
| Description check | ✅ Passed | The description comprehensively relates the changeset scope, implementation details, design decisions, validation steps, and references to roadmap item 11.1.2. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Testing (Overall) | ✅ Passed | New functionality is guarded by substantive, rigorous tests that directly exercise behaviour. Tests use explicit assertions on localiser interaction recording, tracing event capture, and parsed out... |
| User-Facing Documentation | ✅ Passed | User-facing documentation is comprehensive and properly addresses all new functionality introduced in roadmap item 11.1.2: LocalizedParse trait and parse_localized_command function are clearly... |
| Developer Documentation | ✅ Passed | All documentation requirements met: developers guide updated with LocalizedParse/parse_localized_command guidance; design documents (cli-localization-design.md §4.2, design.md) reflect changes; roa... |
| Module-Level Documentation | ✅ Passed | All Rust modules in this PR carry docstrings explaining purpose, utility, function, and component relationships. Primary module parse.rs documents its role connecting parser implementations, comm... |
| Testing (Unit And Behavioural) | ✅ Passed | Tests comprehensively cover unit-level behaviour, edge cases and error paths: 8 unit tests exercise trait methods, error localisation, panic contracts, and identifier coverage; 2 trybuild tests ver... |
| Testing (Property / Proof) | ✅ Passed | The PR introduces documented hard constraints (identifier semantics, output preservation, circular dependency avoidance) but does not establish algorithmic invariants over input ranges, state order... |
| Testing (Compile-Time / Ui) | ✅ Passed | Trybuild tests comprehensively verify compile-time contracts (blanket trait availability and Parser bound enforcement) with meaningful expected diagnostics. Snapshot tests encode help-text/error-ou... |
| Unit Architecture | ✅ Passed | PR maintains clear separation of concerns: parse-localized functions are queries with explicit error handling and warn-level tracing side-effects; LocalizeCmd trait is a documented command with #[m... |
| Domain Architecture | ✅ Passed | LocalizedParse and parse_localized_command properly segregate clap framework concerns within a dedicated adapter module (clap_command/). Domain logic remains framework-agnostic; these utilities are... |
| Observability | ✅ Passed | Observability properly implemented: warn-level tracing at translation fallback boundary with stable fields (identifier, error_kind, locale), comprehensive test coverage validating event emission, a... |
| Security And Privacy | ✅ Passed | No secrets, injection risks, unsafe code, or privacy concerns detected. Logging emits only metadata (identifier, error_kind, locale), test credentials are fake, argument handling is type-safe throu... |
| Performance And Resource Use | ✅ Passed | Changes avoid performance regressions. parse_localized_command and LocalizedParse are thin wrappers around clap parsing with error-path-only allocations. All string/HashMap allocations are bounded... |
| Concurrency And State | ✅ Passed | No shared mutable state, async code, locks held across operations, or concurrency hazards introduced. All state is owned locally by test-local localizer instances; Localizer trait enforces Send+Syn... |
| Architectural Complexity And Maintainability | ✅ Passed | LocalizedParse trait and parse_localized_command function justify their introduction: they remove 81 lines of example-specific boilerplate, establish a crate-level contract with clear reuse paths (... |
| Rust Compiler Lint Integrity | ✅ Passed | No lint suppressions hiding unused code, all imports used, dead code properly removed, no excessive cloning; module structure appropriately scoped. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait
Comment @coderabbitai help to get the list of available commands and usage tips.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait
branch
from
ab73ff3 to
660267f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait
branch
from
90bbb5e to
2c6b597
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
|
@coderabbitai Have the following now been resolved? ❌ Failed checks (1 warning)
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "try_parse_localized|parse_localized_command|LocalizedParse" docs/users-guide.mdLength of output: 823
The document explicitly distinguishes the two approaches by catalogue-key root: use |
…1.2) Add the ExecPlan for roadmap 11.1.2: promote the example's inherent `try_parse_localized*` methods into a crate-level `LocalizedParse: clap::Parser` blanket trait, backed by a base-agnostic public `parse_localized_command` free function so custom-base (`with_base`) applications also drop the copy-pasted error-localization glue. The plan captures the community-of-experts review outcomes: the free-fn primitive (forward-compatible with 11.1.3's derived identifiers), the panic-as-contract decision (ADR-006 amendment), preservation of the `with_cmd` enrichment on the from_arg_matches error path, NoOp transparency, and the identifier-coverage test interpretation. Status: DRAFT — awaiting approval before implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expose `parse_localized_command` as the base-agnostic parsing primitive and add the `LocalizedParse` blanket trait for default-base consumers. Both paths preserve localized clap errors and return raw `ArgMatches` where callers need them for downstream merge logic. Add integration coverage for successful parsing, match preservation, no-op transparency, `from_arg_matches` command enrichment, identifier coverage through `message_id_for`, and the documented panic contract for invalid Fluent roots.
Replace the example's copied parsing glue with the crate-level `parse_localized_command` primitive while preserving the custom `hello_world.cli` identifier base. Remove `ParsedCommandLine` and the inherent `try_parse_localized*` methods so the example no longer shadows the promoted blanket trait API.
Update user, developer, design, ADR, roadmap, and example documentation for `LocalizedParse` and `parse_localized_command`. Mark roadmap item 11.1.2 as complete and record final validation in the execplan. Strengthen the hello world localized help tests so display requests assert successful exit status, stdout output, and empty stderr for help and version requests.
Add the completed 11.1.2 decisions, findings, progress, validation, and CodeRabbit observation to the roadmap so it reflects the implemented localized parsing surface.
Set the 11.1.2 execplan status to complete and record that the roadmap now carries the same decisions, findings, progress, validation, and CodeRabbit observations.
Add observable warnings when clap-error localization falls back to the stock message, including the missing identifier, error kind, and locale context when the localizer exposes one. Add trybuild coverage for the `LocalizedParse: clap::Parser` contract and document why Fluent-unsafe identifiers remain runtime panic coverage until derive-time identifier validation lands. Update the changelog, README signposts, and localization design notes for the promoted parsing APIs. Apply the repository Markdown formatter output required for the documentation changes.
Add user-guide examples for the zero-configuration `LocalizedParse` methods, including environment parsing, supplied-argument parsing, and the matches-returning variant. Clarify that the trait uses the command `bin_name` as its catalogue root, while `parse_localized_command` remains the custom-base path for callers that need a different message-id namespace.
lodyai
Bot
force-pushed
the
11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait
branch
from
4383c9c to
12b9064
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Add runtime coverage for the warn-level clap error fallback event so the stable `identifier`, `error_kind`, and `locale` fields are asserted. Also strengthen the parse success path by proving translated command metadata lookups are used with a caller-provided catalogue base. Expand the localized parsing module documentation to explain how the parse helpers relate to `LocalizeCmd` and `Localizer`, and when to use the free function versus the blanket trait.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Use Oxford spelling in the localized parse module documentation. Extract the non-trivial localized parse test localizers into `rstest` fixtures while keeping the tests as separate scenarios. The reviewed tests exercise different behaviours, so avoid merging them into a single table that would obscure the assertions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document the warn-level tracing emitted when clap error localization falls back to stock output. Add module docs to the localized parse trybuild fixtures so their compile-time contract purpose is explicit. Add user-guide migration guidance for callers moving from direct `LocalizeCmd` command parsing to `LocalizedParse` or `parse_localized_command`.
|
@coderabbitai Have the following now been resolved? ortho_config/tests/localized_parse.rs (1)
Refactor sketch+use rstest::{fixture, rstest};
+
+#[fixture]
+fn fallback_localizer() -> TranslatedLocalizer {
+ TranslatedLocalizer::fallback(langid!("fr-FR"))
+}
+
+#[rstest]
+fn missing_subcommand_reports_expected_warning_fields(
+ fallback_localizer: TranslatedLocalizer,
+) {
+ // shared capture + parse + assert flow
+}As per coding guidelines, Rust tests must use 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Sources: Coding guidelines, Learnings |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "rstest|fixture|#\[rstest\]|TranslatedLocalizer|NoOpLocalizer|fallback_localizer" ortho_config/tests/localized_parse.rs | head -60
echo "---"
sed -n '200,360p' ortho_config/tests/localized_parse.rsLength of output: 8555
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Implements roadmap item 11.1.2 — promote
try_parse_localized*from thehello_worldexample into crate-level localized parsing APIs inortho_config.This adds:
LocalizedParse: clap::Parser, a blanket extension trait for default-base localized parsing.parse_localized_command, the base-agnostic primitive for callers that already localized aclap::CommandwithLocalizeCmd::with_base.hello_worldmigration to the public parsing primitive while preserving itshello_world.clicatalogue base.Plan document:
docs/execplans/11-1-2-promote-try-parse-localized-to-a-generic-blanket-trait.md. Design source:docs/cli-localization-design.md§4.2.Review Feedback Addressed
CHANGELOG.mdentries forLocalizedParseandparse_localized_command.LocalizedParse: clap::Parsercompile-time contract.identifier,error_kind, andlocalefields.Key Design Decisions
parse_localized_commandas the load-bearing primitive and keepLocalizedParseas thin default-base convenience.hello_worldexample on its existingwith_base("hello_world.cli")catalogue..with_cmd(&command)enrichment on thefrom_arg_matcheserror path.Validation
make fmtmake check-fmtmake lintmake testmake markdownlintmake nixieReferences
docs/roadmap.md§11.1.2 (requires 11.1.1, shipped in Promote LocalizeCmd to a public extension trait (11.1.1) #348)docs/cli-localization-design.md§4.2