Skip to content

Commit a129911

Browse files
Copilotjosecelano
andcommitted
feat: standardize error structures between commands
- Add error structure template to error handling guide - Standardize Create command errors (add tips, convert to struct-style) - Add error grouping comments to both Create and Destroy commands - Update all error construction sites and test patterns - All tests pass and linter checks succeed Co-authored-by: josecelano <[email protected]>
1 parent 7e8fac8 commit a129911

File tree

7 files changed

+237
-41
lines changed

7 files changed

+237
-41
lines changed

docs/contributing/error-handling.md

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,169 @@ match FileLock::acquire(&path, timeout) {
574574
- ✅ Platform-aware guidance included
575575
- ✅ Users control verbosity level
576576

577+
## 📐 Error Structure Template
578+
579+
When defining new error types, use this template to ensure consistency:
580+
581+
```rust
582+
use thiserror::Error;
583+
use std::path::PathBuf;
584+
585+
#[derive(Debug, Error)]
586+
pub enum YourCommandError {
587+
// ===== File/Configuration Errors =====
588+
589+
/// Brief description of when this error occurs
590+
///
591+
/// More detailed explanation if needed.
592+
/// Use `.help()` for detailed troubleshooting steps.
593+
#[error("Clear error message with context: {path}
594+
Tip: Brief actionable hint - command example if applicable")]
595+
ConfigFileNotFound {
596+
/// Path to the missing file
597+
path: PathBuf,
598+
},
599+
600+
/// Brief description
601+
#[error("Error message with multiple context values: '{path}' as {format}: {source}
602+
Tip: Validate format with: command --check {path}")]
603+
ParsingFailed {
604+
/// Path to the file
605+
path: PathBuf,
606+
/// Expected format
607+
format: String,
608+
/// Original parsing error
609+
#[source]
610+
source: SomeError,
611+
},
612+
613+
// ===== Operation Errors =====
614+
615+
/// Brief description
616+
///
617+
/// Explanation of when this occurs.
618+
#[error("Operation '{operation}' failed for '{name}': {source}
619+
Tip: Check logs with: --verbose or --log-output file-and-stderr")]
620+
OperationFailed {
621+
/// Name of the resource
622+
name: String,
623+
/// Operation being performed
624+
operation: String,
625+
/// Underlying error
626+
#[source]
627+
source: Box<dyn std::error::Error + Send + Sync>,
628+
},
629+
630+
// Add more error variants as needed, grouped by category
631+
}
632+
633+
impl YourCommandError {
634+
/// Get detailed troubleshooting guidance for this error
635+
///
636+
/// This method provides comprehensive troubleshooting steps that can be
637+
/// displayed to users when they need more help resolving the error.
638+
///
639+
/// # Example
640+
///
641+
/// ```rust
642+
/// if let Err(e) = some_operation() {
643+
/// eprintln!("Error: {e}");
644+
/// if verbose {
645+
/// eprintln!("\nTroubleshooting:\n{}", e.help());
646+
/// }
647+
/// }
648+
/// ```
649+
#[must_use]
650+
pub fn help(&self) -> &'static str {
651+
match self {
652+
Self::ConfigFileNotFound { .. } => {
653+
"Configuration File Not Found - Detailed Troubleshooting:
654+
655+
1. Check file path is correct
656+
- Verify path spelling: ls -la <path>
657+
- Use absolute or relative paths correctly
658+
659+
2. Verify file permissions
660+
- Check read permissions: ls -l <path>
661+
- Fix if needed: chmod 644 <path>
662+
663+
3. Common solutions
664+
- Create the file if missing
665+
- Check current directory: pwd
666+
- Provide correct path in arguments
667+
668+
For more information, see the documentation."
669+
}
670+
671+
Self::ParsingFailed { .. } => {
672+
"Parsing Failed - Detailed Troubleshooting:
673+
674+
1. Validate syntax
675+
- Use appropriate validator tool
676+
- Check for common syntax errors
677+
678+
2. Verify format matches expectation
679+
- Check file extension
680+
- Validate structure
681+
682+
For more information, see format documentation."
683+
}
684+
685+
Self::OperationFailed { .. } => {
686+
"Operation Failed - Detailed Troubleshooting:
687+
688+
1. Check system state
689+
- Verify resources are available
690+
- Check permissions
691+
692+
2. Review logs for details
693+
- Run with --verbose
694+
- Check log output
695+
696+
For persistent issues, contact support."
697+
}
698+
}
699+
}
700+
}
701+
702+
#[cfg(test)]
703+
mod tests {
704+
use super::*;
705+
706+
#[test]
707+
fn it_should_have_help_for_all_variants() {
708+
// Create instances of all error variants
709+
let errors: Vec<YourCommandError> = vec![
710+
// ... create test instances
711+
];
712+
713+
for error in errors {
714+
let help = error.help();
715+
assert!(!help.is_empty(), "Help text should not be empty");
716+
assert!(
717+
help.contains("Troubleshooting") || help.len() > 50,
718+
"Help should contain actionable guidance"
719+
);
720+
}
721+
}
722+
723+
#[test]
724+
fn it_should_display_context_in_errors() {
725+
// Test that context fields appear in error messages
726+
}
727+
}
728+
```
729+
730+
### Template Guidelines
731+
732+
1. **Group related errors** with section comments (e.g., `// ===== File Errors =====`)
733+
2. **Always include context fields** (paths, names, IDs) relevant to the error
734+
3. **Use `#[source]`** for all wrapped errors to preserve error chains
735+
4. **Add brief tips** in error messages using `\nTip:` format
736+
5. **Document each variant** with rustdoc comments explaining when it occurs
737+
6. **Implement `.help()`** with detailed troubleshooting for each variant
738+
7. **Write comprehensive tests** covering all variants and help text
739+
577740
## 📋 Error Review Checklist
578741

579742
When reviewing error handling code, verify:
@@ -589,6 +752,7 @@ When reviewing error handling code, verify:
589752
- [ ] **Pattern Matching**: Can callers handle different error cases appropriately?
590753
- [ ] **Unwrap/Expect**: Is `unwrap()` avoided in favor of `expect()` with descriptive messages?
591754
- [ ] **Consistency**: Does the error follow project conventions?
755+
- [ ] **Error Grouping**: Are related errors grouped with section comments?
592756

593757
## 🔗 Related Documentation
594758

src/presentation/commands/create/config_loader.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ impl ConfigLoader {
9191
config
9292
.clone()
9393
.to_environment_params()
94-
.map_err(CreateSubcommandError::ConfigValidationFailed)?;
94+
.map_err(|source| CreateSubcommandError::ConfigValidationFailed { source })?;
9595

9696
Ok(config)
9797
}
@@ -217,7 +217,7 @@ mod tests {
217217

218218
assert!(result.is_err());
219219
match result.unwrap_err() {
220-
CreateSubcommandError::ConfigValidationFailed(_) => {
220+
CreateSubcommandError::ConfigValidationFailed { .. } => {
221221
// Expected - validation should catch invalid environment name
222222
}
223223
other => panic!("Expected ConfigValidationFailed, got: {other:?}"),
@@ -246,7 +246,7 @@ mod tests {
246246

247247
assert!(result.is_err());
248248
match result.unwrap_err() {
249-
CreateSubcommandError::ConfigValidationFailed(_) => {
249+
CreateSubcommandError::ConfigValidationFailed { .. } => {
250250
// Expected - validation should catch missing SSH keys
251251
}
252252
other => panic!("Expected ConfigValidationFailed, got: {other:?}"),

src/presentation/commands/create/errors.rs

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,25 @@ impl std::fmt::Display for ConfigFormat {
3131
/// troubleshooting and user feedback.
3232
#[derive(Debug, Error)]
3333
pub enum CreateSubcommandError {
34+
// ===== Configuration File Errors =====
35+
3436
/// Configuration file not found
35-
#[error("Configuration file not found: {path}")]
37+
///
38+
/// The specified configuration file does not exist or is not accessible.
39+
/// Use `.help()` for detailed troubleshooting steps.
40+
#[error("Configuration file not found: {path}
41+
Tip: Check that the file path is correct: ls -la {path}")]
3642
ConfigFileNotFound {
3743
/// Path to the missing configuration file
3844
path: PathBuf,
3945
},
4046

4147
/// Failed to parse configuration file
42-
#[error("Failed to parse configuration file '{path}' as {format}")]
48+
///
49+
/// The configuration file exists but could not be parsed in the expected format.
50+
/// Use `.help()` for detailed troubleshooting steps.
51+
#[error("Failed to parse configuration file '{path}' as {format}: {source}
52+
Tip: Validate {format} syntax with: jq . < {path}")]
4353
ConfigParsingFailed {
4454
/// Path to the configuration file
4555
path: PathBuf,
@@ -51,28 +61,44 @@ pub enum CreateSubcommandError {
5161
},
5262

5363
/// Configuration validation failed
54-
#[error("Configuration validation failed")]
55-
ConfigValidationFailed(
64+
///
65+
/// The configuration file was parsed successfully but contains invalid values.
66+
/// Use `.help()` for detailed troubleshooting steps.
67+
#[error("Configuration validation failed: {source}
68+
Tip: Review the validation error and fix the configuration file")]
69+
ConfigValidationFailed {
5670
/// Underlying validation error from domain layer
5771
#[source]
58-
crate::application::command_handlers::create::config::CreateConfigError,
59-
),
72+
source: crate::application::command_handlers::create::config::CreateConfigError,
73+
},
74+
75+
// ===== Command Execution Errors =====
6076

6177
/// Command execution failed
62-
#[error("Create command execution failed")]
63-
CommandFailed(
78+
///
79+
/// The create operation failed during execution after validation passed.
80+
/// Use `.help()` for detailed troubleshooting steps.
81+
#[error("Create command execution failed: {source}
82+
Tip: Check logs with --log-output file-and-stderr for detailed error information")]
83+
CommandFailed {
6484
/// Underlying command handler error
6585
#[source]
66-
CreateCommandHandlerError,
67-
),
86+
source: CreateCommandHandlerError,
87+
},
88+
89+
// ===== Template Generation Errors =====
6890

6991
/// Template generation failed
70-
#[error("Template generation failed")]
71-
TemplateGenerationFailed(
92+
///
93+
/// Failed to generate template configuration or files.
94+
/// Use `.help()` for detailed troubleshooting steps.
95+
#[error("Template generation failed: {source}
96+
Tip: Check that you have write permissions in the target directory")]
97+
TemplateGenerationFailed {
7298
/// Underlying template generation error from domain layer
7399
#[source]
74-
crate::application::command_handlers::create::config::CreateConfigError,
75-
),
100+
source: crate::application::command_handlers::create::config::CreateConfigError,
101+
},
76102
}
77103

78104
impl CreateSubcommandError {
@@ -150,10 +176,10 @@ Example valid configuration:
150176
For more information, see the configuration documentation."
151177
}
152178
},
153-
Self::ConfigValidationFailed(inner) | Self::TemplateGenerationFailed(inner) => {
154-
inner.help()
179+
Self::ConfigValidationFailed { source } | Self::TemplateGenerationFailed { source } => {
180+
source.help()
155181
}
156-
Self::CommandFailed(inner) => inner.help(),
182+
Self::CommandFailed { source } => source.help(),
157183
}
158184
}
159185
}
@@ -228,13 +254,13 @@ mod tests {
228254
format: ConfigFormat::Json,
229255
source: Box::new(std::io::Error::new(std::io::ErrorKind::InvalidData, "test")),
230256
},
231-
CreateSubcommandError::ConfigValidationFailed(
232-
CreateConfigError::InvalidEnvironmentName(EnvironmentNameError::InvalidFormat {
257+
CreateSubcommandError::ConfigValidationFailed {
258+
source: CreateConfigError::InvalidEnvironmentName(EnvironmentNameError::InvalidFormat {
233259
attempted_name: "test".to_string(),
234260
reason: "invalid".to_string(),
235261
valid_examples: vec!["dev".to_string()],
236262
}),
237-
),
263+
},
238264
];
239265

240266
for error in errors {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ fn execute_create_command(
139139
output.progress("Validating configuration and creating environment...");
140140

141141
#[allow(clippy::manual_inspect)]
142-
command_handler.execute(config).map_err(|err| {
143-
let error = CreateSubcommandError::CommandFailed(err);
142+
command_handler.execute(config).map_err(|source| {
143+
let error = CreateSubcommandError::CommandFailed { source };
144144
report_error(output, &error);
145145
error
146146
})
@@ -298,7 +298,7 @@ mod tests {
298298
assert!(result2.is_err(), "Second create should fail");
299299

300300
match result2.unwrap_err() {
301-
CreateSubcommandError::CommandFailed(_) => {
301+
CreateSubcommandError::CommandFailed { .. } => {
302302
// Expected - environment already exists
303303
}
304304
other => panic!("Expected CommandFailed, got: {other:?}"),
@@ -503,7 +503,7 @@ mod tests {
503503
assert!(result2.is_err(), "Second execution should fail");
504504

505505
match result2.unwrap_err() {
506-
CreateSubcommandError::CommandFailed(_) => {
506+
CreateSubcommandError::CommandFailed { .. } => {
507507
// Expected
508508
}
509509
other => panic!("Expected CommandFailed, got: {other:?}"),

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ pub fn handle_template_generation(output_path: &Path) -> Result<(), CreateSubcom
4646
.block_on(async {
4747
EnvironmentCreationConfig::generate_template_file(output_path)
4848
.await
49-
.map_err(CreateSubcommandError::TemplateGenerationFailed)
49+
.map_err(|source| CreateSubcommandError::TemplateGenerationFailed { source })
5050
})?;
5151

5252
output.success(&format!(

src/presentation/commands/create/tests/integration.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ fn it_should_reject_invalid_environment_name() {
8787

8888
assert!(result.is_err(), "Should fail for invalid environment name");
8989
match result.unwrap_err() {
90-
create::CreateSubcommandError::ConfigValidationFailed(_) => {
90+
create::CreateSubcommandError::ConfigValidationFailed { .. } => {
9191
// Expected
9292
}
9393
other => panic!("Expected ConfigValidationFailed, got: {other:?}"),
@@ -103,7 +103,7 @@ fn it_should_reject_missing_ssh_keys() {
103103

104104
assert!(result.is_err(), "Should fail for missing SSH keys");
105105
match result.unwrap_err() {
106-
create::CreateSubcommandError::ConfigValidationFailed(_) => {
106+
create::CreateSubcommandError::ConfigValidationFailed { .. } => {
107107
// Expected
108108
}
109109
other => panic!("Expected ConfigValidationFailed, got: {other:?}"),
@@ -124,7 +124,7 @@ fn it_should_reject_duplicate_environment() {
124124
assert!(result2.is_err(), "Second create should fail");
125125

126126
match result2.unwrap_err() {
127-
create::CreateSubcommandError::CommandFailed(_) => {
127+
create::CreateSubcommandError::CommandFailed { .. } => {
128128
// Expected - environment already exists
129129
}
130130
other => panic!("Expected CommandFailed, got: {other:?}"),

0 commit comments

Comments
 (0)