Implement comprehensive error message improvements with validation

docs/ROADMAP.md

@@ -279,6 +279,18 @@ Dates are a freeform field, with some common conventions. We need to implement p
    - Basic support exists
    - Complete implementation needed
 
+## Known Issues / Technical Debt
+
+### Error Handling and Validation (PR #16)
+The following validation logic branches need dedicated unit test coverage:
+- [ ] Individual validation (empty names check) - Currently only covered by integration tests
+- [ ] Family validation (empty family check) - Missing unit test for all three fields empty
+- [ ] Submitter validation (missing NAME field) - No dedicated unit test for validation branch
+- [ ] Repository validation (missing NAME field) - No test verifying ValidationError generation
+- [ ] Multimedia validation (missing FILE field) - No test verifying MissingRequiredField error
+
+Note: These validation features work correctly and are tested via integration tests, but lack isolated unit tests for better maintainability.
+
 ## Future Enhancements
 
 ### Version 0.2.0 Goals
@@ -290,7 +302,7 @@ Dates are a freeform field, with some common conventions. We need to implement p
 - [x] Complete NOTE record parsing (all GEDCOM 5.5.1 fields)
 - [x] Complete OBJE record parsing (all GEDCOM 5.5.1 fields)
 - [x] Complete REPO record parsing (all GEDCOM 5.5.1 fields)
-- [ ] Improved error messages
+- [x] Improved error messages (enhanced error types, validation warnings, better formatting)
 - [x] Performance optimizations for large files (minimal overhead for new record types)
 
 ### Version 0.3.0 Goals

src/error.rs

@@ -13,29 +13,136 @@ pub enum GedcomError {
     /// Error parsing a specific line or record
     ParseError {
         line_number: Option<usize>,
+        record_type: Option<String>,
+        field: Option<String>,
         message: String,
+        context: Option<String>,
     },
 
     /// Invalid GEDCOM structure (e.g., missing required fields)
-    InvalidStructure(String),
+    InvalidStructure {
+        record_xref: Option<String>,
+        message: String,
+    },
+
+    /// Validation error for a specific field
+    ValidationError {
+        record_type: String,
+        record_xref: Option<String>,
+        field: String,
+        message: String,
+    },
+
+    /// Character encoding issues during file reading
+    EncodingError {
+        declared_encoding: String,
+        detected_encoding: Option<String>,
+        message: String,
+        had_errors: bool,
+    },
+
+    /// Required field is missing from a record
+    MissingRequiredField {
+        record_type: String,
+        record_xref: Option<String>,
+        field: String,
+    },
 }
 
 impl fmt::Display for GedcomError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             GedcomError::Io(err) => write!(f, "I/O error: {}", err),
-            GedcomError::FileNotFound(path) => write!(f, "File not found: {}", path),
+            GedcomError::FileNotFound(path) => {
+                writeln!(f, "File not found: '{}'", path)?;
+                write!(
+                    f,
+                    "  Hint: Check that the file path is correct and the file exists"
+                )
+            }
             GedcomError::ParseError {
                 line_number,
+                record_type,
+                field,
                 message,
+                context,
             } => {
+                write!(f, "Parse error")?;
                 if let Some(line) = line_number {
-                    write!(f, "Parse error at line {}: {}", line, message)
-                } else {
-                    write!(f, "Parse error: {}", message)
+                    write!(f, " at line {}", line)?;
+                }
+                if let Some(rec_type) = record_type {
+                    write!(f, " in {} record", rec_type)?;
                 }
+                if let Some(fld) = field {
+                    write!(f, ", field '{}'", fld)?;
+                }
+                write!(f, ": {}", message)?;
+                if let Some(ctx) = context {
+                    write!(f, "\n  Context: {}", ctx)?;
+                }
+                Ok(())
+            }
+            GedcomError::InvalidStructure {
+                record_xref,
+                message,
+            } => {
+                write!(f, "Invalid GEDCOM structure")?;
+                if let Some(xref) = record_xref {
+                    write!(f, " in record {}", xref)?;
+                }
+                write!(f, ": {}", message)
+            }
+            GedcomError::ValidationError {
+                record_type,
+                record_xref,
+                field,
+                message,
+            } => {
+                write!(f, "Validation error in {} record", record_type)?;
+                if let Some(xref) = record_xref {
+                    write!(f, " ({})", xref)?;
+                }
+                write!(f, ", field '{}': {}", field, message)
+            }
+            GedcomError::EncodingError {
+                declared_encoding,
+                detected_encoding,
+                message,
+                had_errors,
+            } => {
+                write!(
+                    f,
+                    "Character encoding error: declared as '{}', ",
+                    declared_encoding
+                )?;
+                if let Some(detected) = detected_encoding {
+                    write!(f, "detected as '{}', ", detected)?;
+                }
+                write!(f, "{}", message)?;
+                if *had_errors {
+                    write!(
+                        f,
+                        "\n  Warning: Some characters could not be converted correctly"
+                    )?;
+                }
+                Ok(())
+            }
+            GedcomError::MissingRequiredField {
+                record_type,
+                record_xref,
+                field,
+            } => {
+                write!(
+                    f,
+                    "Missing required field '{}' in {} record",
+                    field, record_type
+                )?;
+                if let Some(xref) = record_xref {
+                    write!(f, " ({})", xref)?;
+                }
+                Ok(())
             }
-            GedcomError::InvalidStructure(msg) => write!(f, "Invalid GEDCOM structure: {}", msg),
         }
     }
 }
@@ -73,34 +180,118 @@ mod tests {
     #[test]
     fn test_file_not_found_display() {
         let err = GedcomError::FileNotFound("test.ged".to_string());
-        assert_eq!(format!("{}", err), "File not found: test.ged");
+        let msg = format!("{}", err);
+        assert!(msg.contains("File not found"));
+        assert!(msg.contains("test.ged"));
     }
 
     #[test]
     fn test_parse_error_with_line_number() {
         let err = GedcomError::ParseError {
             line_number: Some(42),
+            record_type: None,
+            field: None,
             message: "Invalid syntax".to_string(),
+            context: None,
         };
-        assert_eq!(format!("{}", err), "Parse error at line 42: Invalid syntax");
+        assert!(format!("{}", err).contains("line 42"));
+        assert!(format!("{}", err).contains("Invalid syntax"));
     }
 
     #[test]
     fn test_parse_error_without_line_number() {
         let err = GedcomError::ParseError {
             line_number: None,
+            record_type: None,
+            field: None,
             message: "Invalid syntax".to_string(),
+            context: None,
         };
-        assert_eq!(format!("{}", err), "Parse error: Invalid syntax");
+        assert!(format!("{}", err).contains("Parse error"));
+        assert!(format!("{}", err).contains("Invalid syntax"));
+    }
+
+    #[test]
+    fn test_parse_error_with_full_context() {
+        let err = GedcomError::ParseError {
+            line_number: Some(42),
+            record_type: Some("INDI".to_string()),
+            field: Some("NAME".to_string()),
+            message: "Invalid name format".to_string(),
+            context: Some("1 NAME /Invalid/Name/".to_string()),
+        };
+        let msg = format!("{}", err);
+        assert!(msg.contains("line 42"));
+        assert!(msg.contains("INDI"));
+        assert!(msg.contains("NAME"));
+        assert!(msg.contains("Invalid name format"));
+        assert!(msg.contains("Context"));
     }
 
     #[test]
     fn test_invalid_structure_display() {
-        let err = GedcomError::InvalidStructure("Missing required field".to_string());
-        assert_eq!(
-            format!("{}", err),
-            "Invalid GEDCOM structure: Missing required field"
-        );
+        let err = GedcomError::InvalidStructure {
+            record_xref: Some("@I1@".to_string()),
+            message: "Missing required field".to_string(),
+        };
+        let msg = format!("{}", err);
+        assert!(msg.contains("Invalid GEDCOM structure"));
+        assert!(msg.contains("@I1@"));
+        assert!(msg.contains("Missing required field"));
+    }
+
+    #[test]
+    fn test_validation_error_display() {
+        let err = GedcomError::ValidationError {
+            record_type: "INDI".to_string(),
+            record_xref: Some("@I1@".to_string()),
+            field: "NAME".to_string(),
+            message: "Name cannot be empty".to_string(),
+        };
+        let msg = format!("{}", err);
+        assert!(msg.contains("Validation error"));
+        assert!(msg.contains("INDI"));
+        assert!(msg.contains("@I1@"));
+        assert!(msg.contains("NAME"));
+        assert!(msg.contains("Name cannot be empty"));
+    }
+
+    #[test]
+    fn test_encoding_error_display() {
+        let err = GedcomError::EncodingError {
+            declared_encoding: "ANSEL".to_string(),
+            detected_encoding: Some("UTF-8".to_string()),
+            message: "Using UTF-8 fallback".to_string(),
+            had_errors: true,
+        };
+        let msg = format!("{}", err);
+        assert!(msg.contains("encoding error"));
+        assert!(msg.contains("ANSEL"));
+        assert!(msg.contains("UTF-8"));
+        assert!(msg.contains("Warning"));
+    }
+
+    #[test]
+    fn test_missing_required_field_display() {
+        let err = GedcomError::MissingRequiredField {
+            record_type: "INDI".to_string(),
+            record_xref: Some("@I1@".to_string()),
+            field: "NAME".to_string(),
+        };
+        let msg = format!("{}", err);
+        assert!(msg.contains("Missing required field"));
+        assert!(msg.contains("NAME"));
+        assert!(msg.contains("INDI"));
+        assert!(msg.contains("@I1@"));
+    }
+
+    #[test]
+    fn test_file_not_found_includes_hint() {
+        let err = GedcomError::FileNotFound("missing.ged".to_string());
+        let msg = format!("{}", err);
+        assert!(msg.contains("File not found"));
+        assert!(msg.contains("missing.ged"));
+        assert!(msg.contains("Hint"));
     }
 
     #[test]
@@ -117,11 +308,17 @@ mod tests {
 
         let err = GedcomError::ParseError {
             line_number: None,
+            record_type: None,
+            field: None,
             message: "test".to_string(),
+            context: None,
         };
         assert!(err.source().is_none());
 
-        let err = GedcomError::InvalidStructure("test".to_string());
+        let err = GedcomError::InvalidStructure {
+            record_xref: None,
+            message: "test".to_string(),
+        };
         assert!(err.source().is_none());
     }