refactor: standardize JsonView render API with Render<T> trait and ViewRenderError#402
Conversation
Introduce ViewRenderError and Render<T> trait in presentation/cli/views/render.rs, providing compile-time enforcement of a consistent render signature across all command view modules. Changes: - Add src/presentation/cli/views/render.rs with ViewRenderError and Render<T> - Re-export both from src/presentation/cli/views/mod.rs - Add impl Render<T> for JsonView to all 13 json_view.rs files Body: Ok(serde_json::to_string_pretty(data)?) - Add impl Render<T> for TextView to all 13 text_view.rs files Body: Ok(TextView::render(data)) — delegates to inherent render method No call sites change. The existing inherent render() methods remain untouched. Trait impls coexist alongside them; inherent methods shadow the trait in UFCS. Proposal #2 will remove the inherent methods and update all call sites to use the trait (adding ? propagation).
…ror through handler chain - Remove inherent pub fn render() from all 13 json_view.rs files - All json_view files now expose rendering exclusively via the Render<T> trait - Add 'use crate::presentation::cli::views::Render;' import to test modules - Update test calls to use trait method (.unwrap() / .expect()) - Add hidden '# use Render;' imports to doc examples in all json_view files - Add 'use crate::presentation::cli::views::Render;' to all 13 handler files - Pattern 1 handlers (configure, destroy, list, release, run, show, test): propagate render error with '?' on JsonView::render call - Pattern 2 handlers (validate, register, purge, render): propagate render error with inner '?' inside progress.result call - Pattern 3 handlers (create/environment, provision): Render import added; call sites unchanged (already use map_err) - Add OutputFormatting variant + From<ViewRenderError> + help() arm to all 11 error types (configure, destroy, list, release, run, show, test, register, purge, validate, render) - Run cargo fmt to fix formatting on all modified files
Both proposals are implemented: - Proposal #1 (ViewRenderError + Render<T> trait): commit 9179886 - Proposal #2 (remove inherent render() methods, propagate errors): commit 80616e4 Update progress tracking, implementation checklists, timeline, review criteria, and overall status to reflect completion.
There was a problem hiding this comment.
Pull request overview
This PR completes the refactor plan to standardize CLI view rendering by introducing a shared Render<T> trait and ViewRenderError, removing JSON view fallback rendering, and propagating render errors through controllers.
Changes:
- Added
ViewRenderError+Render<T>trait and re-exported them from the views module. - Migrated command
JsonView/TextViewtypes to implementRender<T>and updated unit/doc tests accordingly. - Updated controllers to call the trait render API for JSON output and propagate formatting errors via new
OutputFormattingerror variants.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/presentation/cli/views/render.rs | Introduces Render<T> and ViewRenderError shared rendering API. |
| src/presentation/cli/views/mod.rs | Exposes render module and re-exports Render / ViewRenderError. |
| src/presentation/cli/views/commands/validate/views/text_view.rs | Implements Render<ValidateDetailsData> for TextView. |
| src/presentation/cli/views/commands/validate/views/json_view.rs | Switches JsonView to Render<ValidateDetailsData> returning Result. |
| src/presentation/cli/views/commands/test/views/text_view.rs | Implements Render<TestResultData> for TextView. |
| src/presentation/cli/views/commands/test/views/json_view.rs | Switches JsonView to Render<TestResultData> returning Result. |
| src/presentation/cli/views/commands/show/views/text_view.rs | Implements Render<EnvironmentInfo> for TextView. |
| src/presentation/cli/views/commands/show/views/json_view.rs | Switches JsonView to Render<EnvironmentInfo> returning Result. |
| src/presentation/cli/views/commands/run/views/text_view.rs | Implements Render<RunDetailsData> for TextView. |
| src/presentation/cli/views/commands/run/views/json_view.rs | Switches JsonView to Render<RunDetailsData> returning Result. |
| src/presentation/cli/views/commands/render/views/text_view.rs | Implements Render<RenderDetailsData> for TextView. |
| src/presentation/cli/views/commands/render/views/json_view.rs | Switches JsonView to Render<RenderDetailsData> returning Result. |
| src/presentation/cli/views/commands/release/views/text_view.rs | Implements Render<ReleaseDetailsData> for TextView. |
| src/presentation/cli/views/commands/release/views/json_view.rs | Switches JsonView to Render<ReleaseDetailsData> returning Result. |
| src/presentation/cli/views/commands/register/views/text_view.rs | Implements Render<RegisterDetailsData> for TextView. |
| src/presentation/cli/views/commands/register/views/json_view.rs | Switches JsonView to Render<RegisterDetailsData> returning Result. |
| src/presentation/cli/views/commands/purge/views/text_view.rs | Implements Render<PurgeDetailsData> for TextView. |
| src/presentation/cli/views/commands/purge/views/json_view.rs | Switches JsonView to Render<PurgeDetailsData> returning Result. |
| src/presentation/cli/views/commands/provision/views/text_view.rs | Implements Render<ProvisionDetailsData> for TextView. |
| src/presentation/cli/views/commands/provision/views/json_view.rs | Switches JsonView to Render<ProvisionDetailsData> returning Result. |
| src/presentation/cli/views/commands/list/views/text_view.rs | Implements Render<EnvironmentList> for TextView. |
| src/presentation/cli/views/commands/list/views/json_view.rs | Switches JsonView to Render<EnvironmentList> returning Result. |
| src/presentation/cli/views/commands/destroy/views/text_view.rs | Implements Render<DestroyDetailsData> for TextView. |
| src/presentation/cli/views/commands/destroy/views/json_view.rs | Switches JsonView to Render<DestroyDetailsData> returning Result. |
| src/presentation/cli/views/commands/create/views/text_view.rs | Implements Render<EnvironmentDetailsData> for TextView. |
| src/presentation/cli/views/commands/create/views/json_view.rs | Switches JsonView to Render<EnvironmentDetailsData> returning Result. |
| src/presentation/cli/views/commands/configure/views/text_view.rs | Implements Render<ConfigureDetailsData> for TextView. |
| src/presentation/cli/views/commands/configure/views/json_view.rs | Switches JsonView to Render<ConfigureDetailsData> returning Result. |
| src/presentation/cli/controllers/validate/handler.rs | Uses Render trait for JSON output and propagates render errors. |
| src/presentation/cli/controllers/validate/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/test/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/test/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/show/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/show/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/run/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/run/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/render/handler.rs | Uses Render trait for JSON output and propagates render errors. |
| src/presentation/cli/controllers/render/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/release/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/release/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/register/handler.rs | Uses Render trait for JSON output and propagates render errors. |
| src/presentation/cli/controllers/register/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/purge/handler.rs | Uses Render trait for JSON output and propagates render errors. |
| src/presentation/cli/controllers/purge/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/provision/handler.rs | Imports Render for trait-based JSON rendering usage. |
| src/presentation/cli/controllers/list/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/list/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/destroy/handler.rs | Propagates JSON render errors via ?. |
| src/presentation/cli/controllers/destroy/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| src/presentation/cli/controllers/create/subcommands/environment/handler.rs | Imports Render and maps JSON render errors into controller error type. |
| src/presentation/cli/controllers/configure/handler.rs | Updates docs and propagates JSON render errors via ?. |
| src/presentation/cli/controllers/configure/errors.rs | Adds OutputFormatting + From<ViewRenderError> conversion and help text. |
| docs/refactors/plans/standardize-json-view-render-api.md | Marks proposals as completed and records completion metadata. |
| /// Implementors transform a DTO (`T`) into a displayable or parseable string. | ||
| /// The `Result` return type is required even for infallible renderers (e.g., [`TextView`](super::commands::configure::views::text_view)) | ||
| /// so that all renderers share a uniform interface and callers can use `?` unconditionally. |
There was a problem hiding this comment.
The intra-doc link for TextView points to super::commands::configure::views::text_view (the module), not the TextView type. This is likely a broken/misleading rustdoc link; consider linking to super::commands::configure::TextView (re-exported) or ...::text_view::TextView instead.
src/presentation/cli/views/render.rs
Outdated
| //! Text renderers always return `Ok` — they do pure string formatting and never fail. | ||
| //! JSON renderers call `serde_json::to_string_pretty` which is infallible on plain | ||
| //! `#[derive(Serialize)]` types, but the `Result` return type models the theoretical | ||
| //! failure path and is consistent with the `create` and `provision` commands that already | ||
| //! return `Result`. |
There was a problem hiding this comment.
The docs claim serde_json::to_string_pretty is “infallible”/the error path is “unreachable” for plain #[derive(Serialize)] types. That’s not guaranteed (e.g., non‑finite floats, non‑string map keys, or custom Serialize impls can still error), so this wording is misleading. Consider rephrasing to say serialization is expected to succeed for these DTOs but errors are still possible and are propagated via ViewRenderError.
Eliminate the asymmetry between JsonView (trait-only render) and TextView (inherent + trait delegation) by inlining all inherent pub fn render() -> String bodies directly into each Render<T> impl. Changes: - Remove inherent pub fn render() from all 13 text_view.rs files - Inline method body into impl Render<T> for TextView, wrapping final expression in Ok(...) and early returns in Ok(...) - list/text_view.rs: keep render_empty/table helpers in inherent impl; only the pub fn render method was removed - show/text_view.rs: trait impl renamed param 'data' -> 'info' to match the inlined body's existing references - Add '?' to all 15 TextView::render() call sites in handler files - Add From<ViewRenderError> impl to CreateEnvironmentCommandError and ProvisionSubcommandError (Pattern 3 handlers not covered by Proposal #2) - Update doc examples: add hidden '# use Render;' imports + .unwrap() - Update tests: add .unwrap() to all TextView::render() calls - Run cargo fmt
Record that commit 1c71cd7 removed inherent pub fn render() from all 13 text_view files, eliminating the JsonView/TextView asymmetry and applying the 'least surprise' principle consistently.
|
ACK 221c998 |
Summary
Implements the full
standardize-json-view-render-apirefactoring plan, plus a follow-up cleanup that emerged from review.Changes
Proposal #1 — Add
ViewRenderErrorandRender<T>trait (91798861)ViewRenderErrorenum (wrapsserde_json::Errorvia#[from]) insrc/presentation/cli/views/render.rsRender<T>trait withfn render(data: &T) -> Result<String, ViewRenderError>src/presentation/cli/views/mod.rsRender<T>for all 13JsonViewstructs (with JSON serialization)Render<T>for all 13TextViewstructs (infallible, returnsOk)Proposal #2 — Remove inherent render() from json_views, propagate errors (
80616e44)pub fn render()methods from 13json_view.rsfiles — rendering is now exclusively via theRender<T>traitunwrap_or_elsefallback dead codeRenderand propagate errors with?OutputFormatting { reason: String }error variant +From<ViewRenderError>impl +help()arm to all 11 command error typesFollow-up — Remove inherent render() from text_views too (
1c71cd7e)Proposals #1 and #2 left
TextViewwith an inherentpub fn render() -> Stringthat delegated to theRender<T>trait impl. This created an asymmetry:JsonViewwas trait-only,TextViewhad an inherent convenience method. Applying the "least surprise" principle:pub fn render()from all 13text_view.rsfilesRender<T>impl, wrapping the final expression inOk(...)?to all 15TextView::render()call sites in handler filesFrom<ViewRenderError>impl toCreateEnvironmentCommandErrorandProvisionSubcommandError(Pattern 3 handlers not covered by Proposal Scaffolding for main app - 8/9 tasks complete (89%) #2).unwrap()Motivation
Before this refactor:
JsonViewstructs returnedStringwith dead fallback code; 2 returnedResult— inconsistentTextViewhad an inherent method that created an asymmetry withJsonViewAfter this refactor:
JsonViewand 13TextViewstructs implement the sameRender<T>traitTesting
cargo run --bin linter all)Related
docs/refactors/plans/standardize-json-view-render-api.mdstandardize-command-view-folder-structure(merged in d9a1c51)