Skip to content

Commit f28262c

Browse files
committed
Merge #82: Eliminate direct console output bypassing UserOutput abstraction
c9e6f07 fix: use steps() method with multi-line strings for numbered list (copilot-swe-agent[bot]) 6a2902d fix: use info_block for numbered list with sub-items (copilot-swe-agent[bot]) 3728e8f feat: eliminate direct console output bypassing UserOutput (copilot-swe-agent[bot]) 6309517 Initial plan (copilot-swe-agent[bot]) Pull request description: The presentation layer directly called `println!()` and `eprintln!()`, bypassing the `UserOutput` service abstraction. This violated the architecture, prevented verbosity control, and made output untestable. ## Changes **Extended UserOutput API** with three new methods: - `blank_line()` - spacing between output sections - `steps()` - automatic numbering for sequential instructions - `info_block()` - multi-line information blocks All methods respect verbosity levels and route output to appropriate channels (stderr for operational messages, stdout for results). **Refactored presentation layer** to eliminate all direct console calls: - `create/subcommands/template.rs` - replaced 11 `println!()` calls with `steps()` and `blank_line()` - `commands/mod.rs` - replaced 3 `eprintln!()` calls in `handle_error()` with UserOutput methods ## Example Before: ```rust println!(); println!("Next steps:"); println!("1. Edit the template file..."); ``` After: ```rust output.blank_line(); output.steps("Next steps:", &[ "Edit the template file...", "Review default values...", ]); ``` Output format preserved. All messages now respect verbosity levels and can be tested/redirected. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Eliminate Direct Console Output Bypassing UserOutput</issue_title> > <issue_description>**Parent Issue**: #63 > **Type**: 🎯 Quick Win > **Impact**: 🟢🟢🟢 High > **Effort**: 🔵🔵 Medium > **Priority**: P0 > **Depends On**: #66 (Proposal 3) > > ## Problem > > The presentation layer directly calls `println!()` and other console output macros, bypassing the `UserOutput` service abstraction: > > ```rust > fn display_success_message(output: &mut UserOutput, output_path: &Path) { > output.success(&format!("Configuration template generated: {}", output_path.display())); > > println!(); // ❌ Bypasses UserOutput > println!("Next steps:"); // ❌ Bypasses UserOutput > println!("1. Edit the template file..."); // ❌ Bypasses UserOutput > // More println! calls... > } > ``` > > **Why this is a problem:** > - Violates abstraction - `UserOutput` should centralize all user-facing output > - No verbosity control - can't suppress based on verbosity level > - Hard to test - can't capture or verify output > - No output redirection - can't redirect to files or alternative formats > > ## Proposed Solution > > Extend `UserOutput` with structured message methods: > > ```rust > impl UserOutput { > /// Display a multi-line information block > pub fn info_block(&mut self, title: &str, lines: &[&str]) { > if self.verbosity >= VerbosityLevel::Normal { > println!(); > println!("{}", title); > for line in lines { > println!("{}", line); > } > } > } > > /// Display a numbered list of steps > pub fn steps(&mut self, title: &str, steps: &[&str]) { > if self.verbosity >= VerbosityLevel::Normal { > println!(); > println!("{}", title); > for (idx, step) in steps.iter().enumerate() { > println!("{}. {}", idx + 1, step); > } > } > } > > /// Display a blank line (for spacing) > pub fn blank_line(&mut self) { > if self.verbosity >= VerbosityLevel::Normal { > println!(); > } > } > } > ``` > > Then refactor handlers: > ```rust > fn display_success_message(output: &mut UserOutput, output_path: &Path) { > output.success(&format!("Configuration template generated: {}", output_path.display())); > output.blank_line(); > output.steps("Next steps:", &[ > "Edit the template file and replace placeholder values...", > "Review default values...", > "Create the environment...", > ]); > } > ``` > > ## Benefits > > - ✅ All output goes through `UserOutput` service > - ✅ Consistent formatting and verbosity control > - ✅ Testable output (can verify messages in tests) > - ✅ Supports future output redirection (files, JSON, etc.) > - ✅ Easy to change output format globally > - ✅ Aligns with project architecture principles > > ## Implementation Checklist > > **Phase 1: Extend UserOutput API** > - [ ] Add `blank_line()` method to `UserOutput` > - [ ] Add `steps()` method for numbered lists > - [ ] Add `info_block()` method for multi-line info blocks > - [ ] Add tests for new methods > - [ ] Document new methods with examples > > **Phase 2: Refactor template subcommand** > - [ ] Identify all `println!()` calls in `create/subcommands/template.rs` > - [ ] Replace with appropriate `UserOutput` methods > - [ ] Verify output format is preserved > - [ ] Update tests to verify output > > **Phase 3: Audit all presentation layer files** > - [ ] Search `src/presentation/` for `println!()`, `print!()`, `eprintln!()`, `eprint!()` > - [ ] Create inventory of direct output calls > - [ ] Replace each with appropriate `UserOutput` method > - [ ] Add new `UserOutput` methods as needed > > **Phase 4: Validation** > - [ ] Run all tests to ensure output is correct > - [ ] Manual testing with different verbosity levels > - [ ] Verify output redirection works (if implemented) > - [ ] Run linter and fix issues > > ## Acceptance Criteria > > - [ ] `UserOutput` has methods for all output patterns used > - [ ] No direct `println!()`, `print!()`, `eprintln!()`, or `eprint!()` calls in `src/presentation/` > - [ ] All output respects verbosity levels > - [ ] Output format is preserved or improved > - [ ] All tests pass: `cargo test presentation` > - [ ] Pre-commit checks pass: `./scripts/pre-commit.sh` > - [ ] Code follows project conventions > > ## Related Documentation > > - [Refactor Plan](https://github.com/torrust/torrust-tracker-deployer/blob/main/docs/refactors/plans/presentation-commands-cleanup.md)</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> - Fixes #68 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). ACKs for top commit: josecelano: ACK c9e6f07 Tree-SHA512: 44202c950a5aa344a24844dab91e6cf11803f720d85345170fb3ee84c461e2ee29097665d817aae2406fa7a709761ef00955ef85f3284ecf55954e02c31676a0
2 parents 9748d33 + c9e6f07 commit f28262c

File tree

3 files changed

+198
-16
lines changed

3 files changed

+198
-16
lines changed

src/presentation/commands/create/subcommands/template.rs

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,19 @@ pub fn handle_template_generation(output_path: &Path) -> Result<(), CreateSubcom
5353
"Configuration template generated: {}",
5454
output_path.display()
5555
));
56-
println!();
57-
println!("Next steps:");
58-
println!("1. Edit the template file and replace placeholder values:");
59-
println!(" - REPLACE_WITH_ENVIRONMENT_NAME: Choose a unique environment name (e.g., 'dev', 'staging')");
60-
println!(" - REPLACE_WITH_SSH_PRIVATE_KEY_PATH: Path to your SSH private key");
61-
println!(" - REPLACE_WITH_SSH_PUBLIC_KEY_PATH: Path to your SSH public key");
62-
println!("2. Review default values:");
63-
println!(" - username: 'torrust' (can be changed if needed)");
64-
println!(" - port: 22 (standard SSH port)");
65-
println!("3. Create the environment:");
66-
println!(
67-
" torrust-tracker-deployer create environment --env-file {}",
68-
output_path.display()
56+
57+
output.blank_line();
58+
59+
output.steps(
60+
"Next steps:",
61+
&[
62+
"Edit the template file and replace placeholder values:\n - REPLACE_WITH_ENVIRONMENT_NAME: Choose a unique environment name (e.g., 'dev', 'staging')\n - REPLACE_WITH_SSH_PRIVATE_KEY_PATH: Path to your SSH private key\n - REPLACE_WITH_SSH_PUBLIC_KEY_PATH: Path to your SSH public key",
63+
"Review default values:\n - username: 'torrust' (can be changed if needed)\n - port: 22 (standard SSH port)",
64+
&format!(
65+
"Create the environment:\n torrust-tracker-deployer create environment --env-file {}",
66+
output_path.display()
67+
),
68+
],
6969
);
7070

7171
Ok(())

src/presentation/commands/mod.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,12 @@ pub fn execute(command: Commands, working_dir: &std::path::Path) -> Result<(), C
119119
/// # }
120120
/// ```
121121
pub fn handle_error(error: &CommandError) {
122-
eprintln!("Error: {error}");
123-
eprintln!("\nFor detailed troubleshooting:");
124-
eprintln!("{}", error.help());
122+
use crate::presentation::user_output::{UserOutput, VerbosityLevel};
123+
124+
let mut output = UserOutput::new(VerbosityLevel::Normal);
125+
let help_text = error.help();
126+
127+
output.error(&format!("{error}"));
128+
output.blank_line();
129+
output.info_block("For detailed troubleshooting:", &[help_text]);
125130
}

src/presentation/user_output.rs

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,86 @@ impl UserOutput {
264264
pub fn data(&mut self, data: &str) {
265265
writeln!(self.stdout_writer, "{data}").ok();
266266
}
267+
268+
/// Display a blank line to stderr (Normal level and above)
269+
///
270+
/// Used for spacing between sections of output to improve readability.
271+
///
272+
/// # Examples
273+
///
274+
/// ```rust
275+
/// use torrust_tracker_deployer_lib::presentation::user_output::{UserOutput, VerbosityLevel};
276+
///
277+
/// let mut output = UserOutput::new(VerbosityLevel::Normal);
278+
/// output.success("Configuration template generated");
279+
/// output.blank_line();
280+
/// output.progress("Starting next steps...");
281+
/// ```
282+
pub fn blank_line(&mut self) {
283+
if self.verbosity >= VerbosityLevel::Normal {
284+
writeln!(self.stderr_writer).ok();
285+
}
286+
}
287+
288+
/// Display a numbered list of steps to stderr (Normal level and above)
289+
///
290+
/// Useful for displaying sequential instructions or action items.
291+
///
292+
/// # Examples
293+
///
294+
/// ```rust
295+
/// use torrust_tracker_deployer_lib::presentation::user_output::{UserOutput, VerbosityLevel};
296+
///
297+
/// let mut output = UserOutput::new(VerbosityLevel::Normal);
298+
/// output.steps("Next steps:", &[
299+
/// "Edit the configuration file",
300+
/// "Review the settings",
301+
/// "Run the deploy command",
302+
/// ]);
303+
/// // Output to stderr:
304+
/// // Next steps:
305+
/// // 1. Edit the configuration file
306+
/// // 2. Review the settings
307+
/// // 3. Run the deploy command
308+
/// ```
309+
pub fn steps(&mut self, title: &str, steps: &[&str]) {
310+
if self.verbosity >= VerbosityLevel::Normal {
311+
writeln!(self.stderr_writer, "{title}").ok();
312+
for (idx, step) in steps.iter().enumerate() {
313+
writeln!(self.stderr_writer, "{}. {}", idx + 1, step).ok();
314+
}
315+
}
316+
}
317+
318+
/// Display a multi-line information block to stderr (Normal level and above)
319+
///
320+
/// Useful for displaying grouped information or detailed messages.
321+
///
322+
/// # Examples
323+
///
324+
/// ```rust
325+
/// use torrust_tracker_deployer_lib::presentation::user_output::{UserOutput, VerbosityLevel};
326+
///
327+
/// let mut output = UserOutput::new(VerbosityLevel::Normal);
328+
/// output.info_block("Configuration options:", &[
329+
/// " - username: 'torrust' (default)",
330+
/// " - port: 22 (default SSH port)",
331+
/// " - key_path: path/to/key",
332+
/// ]);
333+
/// // Output to stderr:
334+
/// // Configuration options:
335+
/// // - username: 'torrust' (default)
336+
/// // - port: 22 (default SSH port)
337+
/// // - key_path: path/to/key
338+
/// ```
339+
pub fn info_block(&mut self, title: &str, lines: &[&str]) {
340+
if self.verbosity >= VerbosityLevel::Normal {
341+
writeln!(self.stderr_writer, "{title}").ok();
342+
for line in lines {
343+
writeln!(self.stderr_writer, "{line}").ok();
344+
}
345+
}
346+
}
267347
}
268348

269349
#[cfg(test)]
@@ -472,4 +552,101 @@ mod tests {
472552
assert!(normal >= VerbosityLevel::Normal);
473553
assert!(normal < VerbosityLevel::Verbose);
474554
}
555+
556+
#[test]
557+
fn it_should_write_blank_line_to_stderr() {
558+
let (mut output, stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Normal);
559+
560+
output.blank_line();
561+
562+
// Verify blank line went to stderr
563+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
564+
assert_eq!(stderr_content, "\n");
565+
566+
// Verify stdout is empty
567+
let stdout_content = String::from_utf8(stdout_buf.lock().unwrap().clone()).unwrap();
568+
assert_eq!(stdout_content, "");
569+
}
570+
571+
#[test]
572+
fn it_should_not_write_blank_line_at_quiet_level() {
573+
let (mut output, _stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Quiet);
574+
575+
output.blank_line();
576+
577+
// Verify no output at Quiet level
578+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
579+
assert_eq!(stderr_content, "");
580+
}
581+
582+
#[test]
583+
fn it_should_write_steps_to_stderr() {
584+
let (mut output, stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Normal);
585+
586+
output.steps(
587+
"Next steps:",
588+
&[
589+
"Edit the configuration file",
590+
"Review the settings",
591+
"Run the deploy command",
592+
],
593+
);
594+
595+
// Verify steps went to stderr with correct formatting
596+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
597+
assert_eq!(
598+
stderr_content,
599+
"Next steps:\n1. Edit the configuration file\n2. Review the settings\n3. Run the deploy command\n"
600+
);
601+
602+
// Verify stdout is empty
603+
let stdout_content = String::from_utf8(stdout_buf.lock().unwrap().clone()).unwrap();
604+
assert_eq!(stdout_content, "");
605+
}
606+
607+
#[test]
608+
fn it_should_not_write_steps_at_quiet_level() {
609+
let (mut output, _stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Quiet);
610+
611+
output.steps("Next steps:", &["Step 1", "Step 2"]);
612+
613+
// Verify no output at Quiet level
614+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
615+
assert_eq!(stderr_content, "");
616+
}
617+
618+
#[test]
619+
fn it_should_write_info_block_to_stderr() {
620+
let (mut output, stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Normal);
621+
622+
output.info_block(
623+
"Configuration options:",
624+
&[
625+
" - username: 'torrust' (default)",
626+
" - port: 22 (default SSH port)",
627+
],
628+
);
629+
630+
// Verify info block went to stderr
631+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
632+
assert_eq!(
633+
stderr_content,
634+
"Configuration options:\n - username: 'torrust' (default)\n - port: 22 (default SSH port)\n"
635+
);
636+
637+
// Verify stdout is empty
638+
let stdout_content = String::from_utf8(stdout_buf.lock().unwrap().clone()).unwrap();
639+
assert_eq!(stdout_content, "");
640+
}
641+
642+
#[test]
643+
fn it_should_not_write_info_block_at_quiet_level() {
644+
let (mut output, _stdout_buf, stderr_buf) = create_test_user_output(VerbosityLevel::Quiet);
645+
646+
output.info_block("Info:", &["Line 1", "Line 2"]);
647+
648+
// Verify no output at Quiet level
649+
let stderr_content = String::from_utf8(stderr_buf.lock().unwrap().clone()).unwrap();
650+
assert_eq!(stderr_content, "");
651+
}
475652
}

0 commit comments

Comments
 (0)