Integrate six-stage Indicatif progress (3-9-1) with OrthoConfig#266
Merged
Integrate six-stage Indicatif progress (3-9-1) with OrthoConfig#266
Conversation
Add a detailed ExecPlan document for roadmap 3.9.1 specifying design, constraints, risks, and staged work for integrating indicatif::MultiProgress reporting with six stages and localized progress feedback. This plan lays out progress configuration, stage model expansion, reporter implementation, manifest refactoring, testing, and documentation updates. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements a six-stage, localized progress reporting model using indicatif::MultiProgress for standard mode, extends manifest and runner pipelines to expose those stages, adds a configurable progress toggle, and updates tests and docs accordingly while preserving accessible static output. Sequence diagram for six-stage pipeline progress reportingsequenceDiagram
actor User
participant Cli
participant Runner as RunnerModule
participant Manifest as ManifestModule
participant Status as StatusReporter
participant Indicatif as IndicatifReporter
User->>Cli: Parse_args_with_progress_and_accessible
Cli->>Runner: run(cli)
alt Accessible_mode
Runner->>Runner: resolve_output_mode(Accessible)
Runner->>Status: create AccessibleReporter
else Standard_mode_with_progress_true
Runner->>Runner: resolve_output_mode(Standard)
Runner->>Indicatif: IndicatifReporter::new()
Indicatif-->>Runner: boxed_as_StatusReporter
Runner->>Status: use IndicatifReporter
else Standard_mode_with_progress_false
Runner->>Status: create SilentReporter
end
User->>Runner: Command_build_or_manifest
Runner->>Runner: generate_ninja(cli, reporter, tool_key)
Runner->>Runner: resolve_manifest_path(cli)
Runner->>Runner: ensure_manifest_exists_or_error(cli, reporter, manifest_path)
alt Manifest_missing
Runner->>Status: report_pipeline_stage(ManifestIngestion)
Runner-->>Cli: Err ManifestNotFound
else Manifest_exists
Runner->>Runner: cli.network_policy()
Runner->>Manifest: from_path_with_policy_and_stage_callback(path, policy, on_stage)
loop Manifest_stage_callbacks
Manifest-->>Runner: on_stage(ManifestLoadStage)
Runner->>Status: report_pipeline_stage(mapped_PipelineStage)
end
Manifest-->>Runner: NetsukeManifest
Runner->>Status: report_pipeline_stage(IrGenerationValidation)
Runner->>Runner: BuildGraph::from_manifest
Runner->>Status: report_pipeline_stage(NinjaSynthesisAndExecution, tool_key)
Runner->>Runner: ninja_gen::generate
Runner-->>Cli: NinjaContent
end
opt Build_or_tool_execution
Runner->>Runner: run_ninja_or_run_ninja_tool
end
Runner->>Status: report_complete(tool_key)
Runner-->>User: Ok
Class diagram for six-stage progress model and reportersclassDiagram
direction LR
class StageNumberError {
<<enum>>
OutOfRange(value u32)
}
class StageNumber {
- value u32
+ new(value u32) Result_StageNumber_StageNumberError
+ get() u32
}
class StageDescription {
- value String
+ new(value String)
+ as_str() &str
}
class LocalizationKey {
- value &'static str
+ new(value &'static str)
+ as_str() &'static str
}
class StatusReporter {
<<interface>>
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class AccessibleReporter {
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class SilentReporter {
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class IndicatifState {
- progress MultiProgress
- bars Vec~ProgressBar~
- descriptions Vec~StageDescription~
- running_index Option~usize~
- completed bool
- is_hidden bool
}
class IndicatifReporter {
- state Mutex~IndicatifState~
+ new() IndicatifReporter
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
- set_stage_state(state IndicatifState, index usize, status_key LocalizationKey, finish_line bool) void
}
class PipelineStage {
<<enum>>
ManifestIngestion
InitialYamlParsing
TemplateExpansion
FinalRendering
IrGenerationValidation
NinjaSynthesisAndExecution
+ ALL [PipelineStage; 6]
+ index() StageNumber
+ from_index(index u32) Option~PipelineStage~
+ description(tool_key Option~LocalizationKey~) StageDescription
}
class ManifestLoadStage {
<<enum>>
ManifestIngestion
InitialYamlParsing
TemplateExpansion
FinalRendering
}
class ManifestModule {
+ from_path_with_policy(path Path, policy NetworkPolicy) NetsukeManifest
+ from_path_with_policy_and_stage_callback(path Path, policy NetworkPolicy, on_stage FnMut_ManifestLoadStage_) NetsukeManifest
+ from_str_named(yaml &str, name &ManifestName, stdlib_config Option~StdlibConfig~) NetsukeManifest
+ from_str_named_with_stage_callback(yaml &str, name &ManifestName, stdlib_config Option~StdlibConfig~, on_stage FnMut_ManifestLoadStage_) NetsukeManifest
}
class RunnerModule {
+ run(cli &Cli) Result
+ handle_build(cli &Cli, args &BuildArgs, reporter &StatusReporter) Result
+ handle_clean(cli &Cli, reporter &StatusReporter) Result
+ handle_graph(cli &Cli, reporter &StatusReporter) Result
+ handle_ninja_tool(cli &Cli, tool &str, tool_key LocalizationKey, reporter &StatusReporter) Result
+ generate_ninja(cli &Cli, reporter &StatusReporter, tool_key Option~LocalizationKey~) NinjaContent
+ ensure_manifest_exists_or_error(cli &Cli, reporter &StatusReporter, manifest_path &Utf8PathBuf) Result
+ load_manifest_with_stage_reporting(manifest_path &Utf8PathBuf, policy NetworkPolicy, reporter &StatusReporter) NetsukeManifest
}
StatusReporter <|.. AccessibleReporter
StatusReporter <|.. SilentReporter
StatusReporter <|.. IndicatifReporter
IndicatifReporter *-- IndicatifState
IndicatifState o-- MultiProgress
IndicatifState o-- ProgressBar
IndicatifState o-- StageDescription
PipelineStage --> StageNumber
PipelineStage --> StageDescription
ManifestModule --> ManifestLoadStage
ManifestModule --> PipelineStage
RunnerModule --> StatusReporter
RunnerModule --> PipelineStage
RunnerModule --> ManifestModule
RunnerModule --> LocalizationKey
StageDescription --> LocalizationKey
StageNumber --> StageNumberError
Flow diagram for progress configuration and reporter selectionflowchart LR
A[Start_run_cli] --> B[Resolve_output_mode]
B --> C{Accessible_mode?}
C -- Yes --> D[Use_AccessibleReporter]
C -- No --> E[Resolve_progress_setting]
subgraph Progress_config_precedence
E --> F[Read_config_file_progress]
F --> G[Read_env_NETSUKE_PROGRESS]
G --> H[Read_CLI_flag_progress]
H --> I{Final_progress_value}
end
I -- true --> J[Use_IndicatifReporter_with_MultiProgress]
I -- false --> K[Use_SilentReporter]
D --> L[Execute_command_build_clean_graph_manifest]
J --> L
K --> L
L --> M[generate_ninja_with_stage_reporting]
M --> N[Manifest_ingestion]
N --> O[Initial_YAML_parsing]
O --> P[Template_expansion]
P --> Q[Final_rendering]
Q --> R[IR_generation_and_validation]
R --> S[Ninja_synthesis_and_execution]
S --> T[Reporter_emits_stage_summaries]
T --> U[Reporter_report_complete]
U --> V[End_run]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
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:
WalkthroughIntroduce a six‑stage, localisable real‑time progress pipeline: add tri‑state CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (flags/options)
participant Runner as Runner
participant Manifest as ManifestLoader
participant Reporter as StatusReporter
participant UI as ProgressUI
CLI->>Runner: start (progress, accessible)
Runner->>Runner: resolve OutputMode & progress
Runner->>Reporter: make_reporter(mode, progress)
Reporter->>UI: init MultiProgress / prepare output
Runner->>Manifest: load_manifest_with_stage_reporting(path, policy)
Manifest->>Reporter: report_stage(stage 1/6, "Manifest Ingestion")
Reporter->>UI: update stage 1
Manifest->>Reporter: report_stage(stage 2/6, "Initial YAML Parsing")
Reporter->>UI: update stage 2
Manifest->>Reporter: report_stage(stage 3/6, "Template Expansion")
Reporter->>UI: update stage 3
Manifest->>Reporter: report_stage(stage 4/6, "Final Rendering")
Reporter->>UI: update stage 4
Runner->>Reporter: report_stage(stage 5/6, "IR Generation & Validation")
Reporter->>UI: update stage 5
Runner->>Reporter: report_stage(stage 6/6, "Ninja Synthesis & Execution")
Reporter->>UI: update stage 6
Runner->>Reporter: report_complete(tool_key)
Reporter->>UI: finalise and emit completion message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
…atif - Integrate indicatif::MultiProgress to display persistent progress summaries for six pipeline stages. - Provide localized, stateful labels for pipeline stages: pending, in progress, done, failed. - Support CLI flag, env var, and config file options to enable or disable standard progress summaries. - Accessible mode produces static, labelled status lines and takes precedence over standard mode. - Add manifest loading stages with explicit callbacks for fine-grained progress reporting. - Update documentation and localization files for progress reporting and new CLI flag `--progress`. - Add behavioral tests for progress output, including localization and failure scenarios. - Replace silent reporter with indicatif reporter when progress summaries are enabled in standard mode. - Ensure accurate stage reporting and graceful fallback on failures. This enhancement improves user feedback during builds with real-time, accessible, and localized progress summaries. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
…-progress - Integrated indicatif::MultiProgress for standard terminal progress reporting. - Implemented six-stage progress model with localized stage/state labels. - Added fallback for non-TTY stderr: emit localized summary lines deterministically. - Configured via OrthoConfig layering with support for CLI args & env variables. - Expanded rstest and rstest-bdd coverage for reporter logic, stages, and failure paths. - Updated documentation and marked roadmap item 3.9.1 as done. - Improved testing helpers to normalize Fluent bidi isolate characters. This enables persistent interactive progress summaries on TTYs while providing stable, testable summary lines in CI and non-interactive environments. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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: Primitive Obsessionsrc/status.rs: What lead to degradation?In this module, 32.4% of all function arguments are primitive types, threshold = 30.0% Why does this problem occur?Code that uses a high degree of built-in, primitives such as integers, strings, floats, lacks a domain language that encapsulates the validation and semantics of function arguments. Primitive Obsession has several consequences: 1) In a statically typed language, the compiler will detect less erroneous assignments. 2) Security impact since the possible value range of a variable/argument isn't retricted. How to fix it?Primitive Obsession indicates a missing domain language. Introduce data types that encapsulate the details and constraints of your domain. For example, instead of int userId, consider User clicked. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# primitive-obsession.ts
-// Problem: It's hard to know what this function does, and what values are valid as parameters.
-function getPopularRepositories(String baseURL, String query, Integer pages, Integer pageSize, String sortorder): Json {
- let pages == null ? 10 : pages
- let pageSize == null ? 10 : pageSize
- return httpClient.get(`${baseURL}?q=${query}&pages=${pages}&pageSize=${pageSize}&sortorder=${sortorder}`)
+// Refactoring: extract the pagination & API logic into a class, and it will
+// attract validation and other logic related to the specific query. It's now
+// easier to use and to maintain the getPopularRepositories function!
+function getPopularRepositories(query: PaginatedRepoQuery): Json {
+ return httpClient.get(query.getURL())
.map(json => json.repositories)
.filter(repository => repositry.stargazersCount > 1000)
} |
This comment was marked as resolved.
This comment was marked as resolved.
- Replaced raw integers and string keys related to pipeline stages with strongly typed structs: `StageNumber`, `StageDescription`, and `LocalizationKey`. - Moved `PipelineStage` enum and related reporting functions to a new module `status_pipeline.rs`. - Updated all `StatusReporter` trait methods and implementations to use the new types instead of raw primitives. - Improved error handling and validation around stage numbers. - Added tests for the new stage modeling types in `status_tests.rs`. - Enhanced localization usage consistency by wrapping keys in `LocalizationKey` type. - Changed calls in `runner` to pass `LocalizationKey` instances instead of plain static string keys. This refactor enhances type safety, readability, and maintainability of the pipeline status reporting subsystem. Co-authored-by: devboxerhub[bot] <devboxerhub[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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/execplans/3-8-1-add-accessible-output-mode.md (1)
175-194:⚠️ Potential issue | 🟡 MinorWrap long bullet lines to 80 columns.
Reflow the bullets in the Outcomes section to comply with wrapping rules.As per coding guidelines, "Markdown paragraphs and bullet points must be wrapped at 80 columns."Proposed fix
-- The `resolve_with` dependency-injection pattern for environment variable - lookup made both unit tests and BDD tests clean and deterministic. -- The `OutputMode` enum and `StatusReporter` trait provide a clean - abstraction that future spinner work (roadmap 3.9) can plug into. -- The plan's staged approach (A: detection, B: CLI, C: status, D: testing) - worked well with checkpoint validation between stages. +- The `resolve_with` dependency-injection pattern for environment variable + lookup made both unit tests and BDD tests clean and deterministic. +- The `OutputMode` enum and `StatusReporter` trait provide a clean + abstraction that future spinner work (roadmap 3.9) can plug into. +- The plan's staged approach (A: detection, B: CLI, C: status, D: testing) + worked well with checkpoint validation between stages. @@ -- `clippy::print_stderr` is denied globally, requiring `writeln!(io::stderr())` - instead of `eprintln!`. The `drop()` wrapper follows the `main.rs` pattern. -- The BDD `Given the environment variable` step already existed in - `manifest/mod.rs` and sets real process env vars. Using "simulated" - env vars in the TestWorld avoided test interference. +- `clippy::print_stderr` is denied globally, requiring `writeln!(io::stderr())` + instead of `eprintln!`. The `drop()` wrapper follows the `main.rs` pattern. +- The BDD `Given the environment variable` step already existed in + `manifest/mod.rs` and sets real process env vars. Using "simulated" + env vars in the TestWorld avoided test interference. @@ -- 4 new files created, 10 files modified. -- 12 unit tests (output_mode), 7 BDD scenarios (accessible_output). -- ~300 net new lines of code (well within the 800-line tolerance). +- 4 new files created, 10 files modified. +- 12 unit tests (output_mode), 7 BDD scenarios (accessible_output). +- ~300 net new lines of code (well within the 800-line tolerance).src/runner/mod.rs (1)
1-419:⚠️ Potential issue | 🟠 MajorSplit this module to bring it under 400 lines.
Decompose
src/runner/mod.rsby moving helper types and/or manifest-stage reporting helpers into submodules so the file length cap is met.
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 30: The Cargo.toml currently pins the indicatif crate to "0.17.11";
update the dependency declaration for indicatif to the latest stable release
"0.18.3" (replace indicatif = "0.17.11" with indicatif = "0.18.3"), then run
cargo update / cargo build to ensure the new version resolves and tests compile;
verify any API changes in code that references indicatif
(symbols/types/functions used) and adjust calls if necessary.
In `@docs/execplans/3-9-1-integrate-indicatif-multi-progress.md`:
- Around line 334-346: Replace the indented command examples with fenced code
blocks that include a language identifier: wrap the first gate sequence (lines
containing "set -o pipefail" and the three make commands: "make check-fmt",
"make lint", "make test") in a ```shell ... ``` block, and wrap the
documentation-gates sequence (the second "set -o pipefail" and "make fmt", "make
markdownlint", "make nixie") in its own ```shell ... ``` block; ensure there are
no stray leading "+" characters and use 'shell' as the language identifier (use
'plaintext' only if any block is non-command text).
- Around line 37-40: The stage name list uses non-Oxford spelling for the
deserialisation stage; update the six-stage pipeline string that currently reads
"Manifest Ingestion, Initial YAML Parsing, Template Expansion, Deserialisation &
Final Rendering, IR Generation & Validation, and Ninja Synthesis & Execution" to
use the -ize spelling ("Deserialization & Final Rendering") so it conforms to
the en-GB-oxendict guideline; search for the exact stage phrase and replace
"Deserialisation" with "Deserialization" wherever the six-stage pipeline is
defined or documented.
In `@docs/netsuke-design.md`:
- Around line 2042-2051: The paragraph describing the new progress reporter is
not wrapped to 80 columns; reflow the text in docs/netsuke-design.md so each
line is <=80 characters while preserving the original content and terminology
(keep references to src/status.rs, indicatif::MultiProgress, the six-stage
model, localized state labels `pending`/`in progress`/`done`/`failed`, stderr
TTY fallback, accessible output behavior, and configuration via OrthoConfig with
`progress: Option<bool>` / `--progress` / `NETSUKE_PROGRESS`). Ensure the
paragraph remains a single Markdown paragraph and follows the project wrapping
rules.
In `@docs/users-guide.md`:
- Around line 606-616: Rewrite the paragraph that begins "Progress summaries are
enabled by default in standard mode." to remove second‑person pronouns and wrap
text to 80 columns; keep the meaning and retain the configuration examples (CLI
flag `--progress true`/`false`, env `NETSUKE_PROGRESS=true`, config `progress =
true`), phrasing it in neutral/impersonal voice (e.g., "Progress summaries are
enabled by default. They can be forced on or off via layered configuration:
...") and ensure all lines in the paragraph and bullet points do not exceed 80
characters.
In `@src/manifest/tests/stages.rs`:
- Around line 19-62: Extract the duplicated tempdir + manifest path setup into
an rstest fixture by adding a function annotated with #[fixture] that creates a
tempfile::TempDir and returns (TempDir, PathBuf) for the Netsukefile path; then
convert the two tests stage_callback_reports_expected_order_for_valid_manifest
and stage_callback_stops_after_parse_failure to #[rstest] and accept the fixture
as a parameter, keeping each test's specific std::fs::write call (the file
contents) inside the test body; ensure you still call
manifest::from_path_with_policy_and_stage_callback with the fixture's PathBuf
and update imports to include rstest::{rstest, fixture} as needed.
In `@src/runner/mod.rs`:
- Around line 109-119: The doc/example for generate_ninja is stale: update every
example call site (e.g., the snippet shown around generate_ninja usage and other
occurrences at lines referenced) to include the new third parameter tool_key
(pass None if you don't have one) so the example signature matches fn
generate_ninja(cli: &Cli, reporter: &dyn StatusReporter, tool_key:
Option<LocalizationKey>) -> Result<NinjaContent>; locate uses of generate_ninja
in docs and tests and change calls like generate_ninja(&cli, &SilentReporter) to
generate_ninja(&cli, &SilentReporter, None).
In `@src/status_pipeline.rs`:
- Around line 6-53: Add a compile-time check to prevent the enum variants,
PipelineStage::ALL, PIPELINE_STAGE_COUNT/PIPELINE_STAGE_TOTAL, and from_index()
from drifting: define a const PIPELINE_STAGE_COUNT: usize =
PipelineStage::ALL.len() and then add a const equality assertion like const
_PIPELINE_STAGE_CHECK: [(); PIPELINE_STAGE_TOTAL as usize] = [();
PIPELINE_STAGE_COUNT]; also change from_index() (or add a small wrapper) to rely
on PIPELINE_STAGE_TOTAL/PIPELINE_STAGE_COUNT (e.g., reject indices >
PIPELINE_STAGE_COUNT) so the match/create logic and the ALL array length are
statically tied together.
In `@src/status_tests.rs`:
- Around line 39-41: In the test function
pipeline_stage_count_matches_stage_array replace the
u32::try_from(...).unwrap_or(0) call with
u32::try_from(PipelineStage::ALL.len()).expect(...) so the conversion failure
surfaces in test output; update the expect message to something descriptive
(e.g., "failed to convert PipelineStage::ALL.len() to u32") and keep the rest of
the assertion against PIPELINE_STAGE_COUNT unchanged.
In `@src/status.rs`:
- Around line 124-143: Clippy warns about drop_non_drop where
drop(writeln!(...)) is used; update each occurrence to use let _ = writeln!(...)
instead so write errors are ignored without calling drop on a Result.
Specifically, in AccessibleReporter::report_stage and
AccessibleReporter::report_complete replace drop(writeln!(io::stderr(),
"{message}")) with let _ = writeln!(io::stderr(), "{message}"), and make the
same change for the other similar methods in this file (the other report_*
blocks noted in the comment) to eliminate the lint.
- Around line 250-269: The Drop impl for IndicatifReporter contains a no-op
borrow `let _ = &state.progress;` (and a similar occurrence later) that should
be removed or explicitly justified; update the drop path in the Impl Drop for
IndicatifReporter to either delete these accidental no-op borrows or replace
them with a short comment explaining the required side-effect (e.g., keeping
borrow alive for ordering or triggering destructor) so the intent is
clear—locate the `drop` method on `IndicatifReporter` and the comparable block
around the other occurrence and remove the meaningless `let _ =
&state.progress;` lines or add a one-line comment explaining why the borrow must
remain.
In `@tests/bdd/helpers/assertions.rs`:
- Around line 10-20: Make normalize_fluent_isolates public (pub fn
normalize_fluent_isolates) in the assertions helper and delete the duplicate
implementations elsewhere; then update the other modules that currently
reimplement it (the manifest_command and localization test modules) to import
and call the single exported normalize_fluent_isolates instead of their local
copies. Ensure the original signature and behavior are preserved and adjust
imports (use ...) in those modules to reference the helper’s
normalize_fluent_isolates.
In `@tests/bdd/steps/manifest_command.rs`:
- Around line 32-37: The test file defines a local normalize_fluent_isolates
function that duplicates the helper in tests/bdd/helpers/assertions.rs; remove
the local function and import/reuse the shared helper instead (export the helper
from assertions.rs if it isn't public) so tests use the single canonical
normalize_fluent_isolates implementation; ensure the test module references the
helper by its exported name and delete the duplicate function in
manifest_command.rs.
Refactor status reporting interfaces and implementations to replace the StageDescription wrapper with plain &str or String parameters. This simplifies the pipeline stage reporting code, removes unnecessary wrapper types, and improves clarity. Impacts include: - Remove StageDescription struct and associated error type. - Change StatusReporter trait methods to accept &str for stage description. - Update implementations (AccessibleReporter, SilentReporter, IndicatifReporter) accordingly. - Adjust PipelineStage::description to return String directly. - Simplify related test code. This refactor retains previous behavior but cleans up types and improves interface consistency for pipeline stage status reporting. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/users-guide.md`:
- Around line 606-623: The documentation uses US spelling and incorrect acronym
casing: replace the word "localized" with British en-GB spelling "localised" in
the standard-mode progress paragraph and the state-prefix line, and change
"continuous integration (CI)" to "Continuous Integration (CI)" (ensuring the
acronym is expanded on first use exactly as "Continuous Integration (CI)").
Update any other occurrences of "localized" or lowercase "continuous
integration" in the shown standard-mode section so they follow en-GB-oxendict
spelling and proper acronym casing.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
This comment was marked as resolved.
This comment was marked as resolved.
Moved manifest path resolution, output path resolution, and manifest existence validation into a new module `runner::path_helpers` to improve code organization and separation of concerns. Additionally, streamlined runner module by delegating these responsibilities to the helper functions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/status_pipeline.rs`:
- Around line 37-38: Update the doc comment for the enum variant
NinjaSynthesisAndExecution to use en-GB spelling: change "synthesize" to
"synthesise" so the comment reads "Stage 6: synthesise Ninja and execute command
intent." Keep the identifier NinjaSynthesisAndExecution unchanged; only edit the
preceding comment text.
…ath string conversion - Changed map_string_or_list from using str::to_string to str::to_owned to avoid unnecessary allocations. - Updated get_target_display_name to use explicit closure and Utf8PathBuf to_string call for clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai Have the following now been resolved? ❌ Not Resolved (8 items) Code structure: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1520 🏁 Script executed: Length of output: 924
Status SummaryComment 4: PIPELINE_STAGE_TOTAL location ✅Resolved - pub(crate) const PIPELINE_STAGE_TOTAL: StageNumber =
StageNumber::new_unchecked(PipelineStage::ALL.len() as u32);It's imported into Comment 5: Manifest callback API ✅Resolved - The API has been collapsed to a single implementation. There are no separate
This matches the reviewer's suggestion to use a trait object and collapse the dual function families. Comment 7: Reporter selection helper ✅Resolved - The three-way match has been extracted into a All code structure comments are now complete. The remaining unresolved items are documentation spelling/style issues (Comments 10-14) requiring en-GB spelling changes. |
Replaced multiple instances of `str::to_string` method calls with `str::to_owned` in test modules to improve semantic clarity and consistency in string conversions without changing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/bdd/types.rs (1)
1-408: 🧹 Nitpick | 🔵 TrivialDecompose this module — file exceeds 400-line limit.
The file is 408 lines, breaching the 400-line ceiling. Extract domain-specific newtypes into separate submodules (e.g.,
types/cli.rs,types/manifest.rs,types/ninja.rs,types/stdlib.rs) and re-export from atypes/mod.rs. This improves maintainability and keeps each submodule focused.As per coding guidelines: "Files must not exceed 400 lines in length … Large modules must be decomposed."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bdd/types.rs` around lines 1 - 408, The module is too large; split it into focused submodules and re-export them: move the define_newtype! macro and any shared imports (e.g., use std::fmt, use std::path::{Path, PathBuf}) into types/mod.rs (or a common prelude), then create files like types/cli.rs, types/manifest.rs, types/ninja.rs, types/stdlib.rs and move related newtypes (e.g., CliArgs, TargetName, PathString, UrlString, ManifestPath, NamesList, TokenList, NinjaFragment, ContentName, HelperName, JobCount, FileContents, etc.) and their impl blocks into the appropriate submodule; ensure PathString::as_path, ManifestPath::as_path, NamesList::iter/to_set, TokenList::to_vec, ContentName::as_str, HelperName, and JobCount::new/value implementations are preserved in their respective files; finally update types/mod.rs to pub use each submodule’s public types so existing call sites keep the same paths.tests/manifest_jinja_tests.rs (1)
1-648: 🛠️ Refactor suggestion | 🟠 MajorDecompose this test module — file exceeds 400 lines
This file is 648 lines, well over the 400-line threshold. Split into focused modules, for example:
manifest_jinja_vars_tests.rs— global vars, env function testsmanifest_jinja_macros_tests.rs— macro registration and argument variantsmanifest_jinja_foreach_tests.rs— foreach expansion, filtering, edge casesmanifest_jinja_rendering_tests.rs— target field rendering, recipe typesExtract shared helpers (
assert_string_or_list_eq,extract_target_*, enums) into a common module or extendtest_support.As per coding guidelines: "Files must not exceed 400 lines in length" and "Large modules must be decomposed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/manifest_jinja_tests.rs` around lines 1 - 648, The test module tests/manifest_jinja_tests.rs is too large (>400 lines); split it into smaller focused test files (e.g., manifest_jinja_vars_tests.rs, manifest_jinja_macros_tests.rs, manifest_jinja_foreach_tests.rs, manifest_jinja_rendering_tests.rs) and move shared helpers and types into a common test helper module (or test_support): extract EnvVar, FieldName, assert_string_or_list_eq, assert_string_or_list_eq_list, extract_target_field/extract_target_names/extract_target_commands, ENV_YAML/ENV_MISSING_YAML, and manifest_yaml usage into that shared module, then update each new test file to import those helpers and keep each file focused on the related tests (e.g., renders_global_vars, renders_env_function into vars tests; registers_manifest_macros and registers_manifest_macro_argument_variants into macros tests; foreach_* tests into foreach tests; renders_target_fields_* and recipe-type tests into rendering tests) so no individual file exceeds the 400-line guideline.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bdd/types.rs`:
- Around line 1-408: The module is too large; split it into focused submodules
and re-export them: move the define_newtype! macro and any shared imports (e.g.,
use std::fmt, use std::path::{Path, PathBuf}) into types/mod.rs (or a common
prelude), then create files like types/cli.rs, types/manifest.rs,
types/ninja.rs, types/stdlib.rs and move related newtypes (e.g., CliArgs,
TargetName, PathString, UrlString, ManifestPath, NamesList, TokenList,
NinjaFragment, ContentName, HelperName, JobCount, FileContents, etc.) and their
impl blocks into the appropriate submodule; ensure PathString::as_path,
ManifestPath::as_path, NamesList::iter/to_set, TokenList::to_vec,
ContentName::as_str, HelperName, and JobCount::new/value implementations are
preserved in their respective files; finally update types/mod.rs to pub use each
submodule’s public types so existing call sites keep the same paths.
In `@tests/manifest_jinja_tests.rs`:
- Around line 1-648: The test module tests/manifest_jinja_tests.rs is too large
(>400 lines); split it into smaller focused test files (e.g.,
manifest_jinja_vars_tests.rs, manifest_jinja_macros_tests.rs,
manifest_jinja_foreach_tests.rs, manifest_jinja_rendering_tests.rs) and move
shared helpers and types into a common test helper module (or test_support):
extract EnvVar, FieldName, assert_string_or_list_eq,
assert_string_or_list_eq_list,
extract_target_field/extract_target_names/extract_target_commands,
ENV_YAML/ENV_MISSING_YAML, and manifest_yaml usage into that shared module, then
update each new test file to import those helpers and keep each file focused on
the related tests (e.g., renders_global_vars, renders_env_function into vars
tests; registers_manifest_macros and registers_manifest_macro_argument_variants
into macros tests; foreach_* tests into foreach tests; renders_target_fields_*
and recipe-type tests into rendering tests) so no individual file exceeds the
400-line guideline.
Replace `ToString::to_string` function references with explicitly typed closures (e.g. `|e: &(dyn std::error::Error + '_)| e.to_string()`) when converting trait objects and non-str Display implementors. Nightly clippy's `str_to_string` lint false-positively flags `ToString::to_string` on `&dyn Error` and `&LocalizedMessage` as `&str`-to-`String` conversions. A plain closure triggers the `redundant_closure_for_method_calls` lint instead. Adding an explicit type annotation to the closure parameter disambiguates the call and satisfies both lints simultaneously. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
leynos
changed the title
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
Changes
docs/execplans/3-9-1-integrate-indicatif-multi-progress.md(aligned with implemented six-stage model).IndicatifReporter) and wired it into the runner when standard output is active and progress is enabled.PipelineStageandManifestLoadStagewith persistent stage lines.--progressflag and integrated OrthoConfig precedence for progress rendering.indicatif = 0.18.4to Cargo.toml and corresponding lock entries.Rationale
Plan of work (high level)
progresssetting and localized CLI/help keys.indicatif::MultiProgressstandard reporter; preserve accessible reporter for accessibility mode.rstest).rstest-bdd) for standard/accessible paths and localization.Acceptance criteria
Validation and evidence
Rollback / recovery
--progresswith a guarded default.Context and references
indicatif.docs/netsuke-design.md,docs/users-guide.md, and existing3-8-1accessibility plan.◳ Generated by DevBoxer ◰
Generated by DevBoxer
📎 Task: https://www.devboxer.com/task/b559494e-78a2-4194-8a26-6cb42cb93d1c