Introduce ErrorRecord dataclass for structured error files - #81
Introduce ErrorRecord dataclass for structured error files#81yoonspark wants to merge 5 commits into
ErrorRecord dataclass for structured error files#81Conversation
- Replace `write_error_file` helper with `ErrorRecord` dataclass that owns read, write, and capture-from-exception logic - Drop the `file` key from the on-disk `.err` schema; the stem is already recoverable from the filename - Write error details into `.setup-failed` sentinel files so setup failures are inspectable beyond just the sentinel's existence
The `.err` stem is derived from the output filename, which does not always recover the original input filename. Store it on the record so `PipelineOutput` can display the input the user recognizes
b53a311 to
ee08cc9
Compare
cswaney
left a comment
cswaney
left a comment
There was a problem hiding this comment.
I like the idea of incorporating the error writing function into a class. This is a clean design.
Might question is whether ErrorRecord and FileError need to be separate classes?
Their fields largely overlap—the differences are path (could be optional), the str vs datetime timestamp, and Pydantic vs dataclass.
Would it work to collapse them into one type with file: str | None, path: str | None, timestamp: datetime? That would let PipelineReport.errors hold the records directly and remove the conversion logic in models.py:625-639.
I think there is also a question of whether path should be set on read/write.
Open to thoughts.
| with pytest.raises(ValueError, match="invalid error record"): | ||
| ErrorRecord.read(path) | ||
|
|
||
| def test_read_without_file_key(self, tmp_path: Path): |
There was a problem hiding this comment.
Duplicates test_write_read_roundtrip_without_file?
There was a problem hiding this comment.
These tests look similar but cover different cases.
test_write_read_roundtrip_without_filewrites the file usingErrorRecord.write(), hence testingErrorRecord's full write-read round trip.test_read_without_file_keywrites the file using rawPath.write_text(), hence testingErrorRecord's read behavior only.
`FileError` duplicated every `ErrorRecord` field, so reading an .err file meant unpacking the record and rebuilding it field by field. It is now a slotted dataclass holding the record plus `path`, the file's location. Timestamps stay raw strings instead of being parsed, so a malformed one no longer discards the whole record.
|
@cswaney Revisiting this PR to integrate your feedback. I updated On a separate but related note, we should also replace other Pydantic models used by |
2 participants
Summary
write_error_filehelper withErrorRecorddataclass that owns read, write, and capture-from-exception logic.setup-failedsentinel files so setup failures are inspectable beyond just the sentinel's existenceTesting
.errfiles contain details in correct format