refactor: extract shared YAMLFile load/save helper#319
Conversation
- Add YAMLFile enum with generic load/save for YAML-backed persistence - Refactor MCSConfig, PackRegistryFile, ProjectIndex, and Lockfile to use it - Remove direct Yams imports from files that no longer need them
bguidolim
left a comment
bguidolim
left a comment
There was a problem hiding this comment.
PR Review Summary
Clean, well-scoped extraction. One important finding, a few optional suggestions. Details in inline comments below.
Strengths
- The
T?+throwsAPI contract is well-designed — nil for missing/empty, throws for corrupt — lets each caller apply its own error policy - Caseless
enumis idiomatic Swift for a stateless function namespace import Yamscorrectly centralized (removed from 3 files, retained in Lockfile.swift)Lockfile.saveintentionally left unchanged — correct decision (header comment)- Thorough test coverage (7 tests) + existing caller tests (68 total) for regression safety
| return try YAMLDecoder().decode(MCSConfig.self, from: content) | ||
| return try YAMLFile.load(MCSConfig.self, from: path) ?? MCSConfig() | ||
| } catch { | ||
| output?.warn("Config file is corrupt (\(path.lastPathComponent)): \(error.localizedDescription)") |
There was a problem hiding this comment.
Important: Loss of error discrimination between "unreadable" and "corrupt".
The original code had two separate do/catch blocks with distinct messages:
String(contentsOf:)failure: "Could not read config file: ..."YAMLDecoder().decode()failure: "Config file is corrupt (...): ..."
After this refactor, both errors hit the single catch and always report "corrupt." If the file exists but is unreadable (permissions, I/O error), the user sees a misleading message. This conflicts with the project's documented missing-vs-corrupt discrimination pattern.
Consider pattern-matching on the error type:
do {
return try YAMLFile.load(MCSConfig.self, from: path) ?? MCSConfig()
} catch is DecodingError {
output?.warn("Config file is corrupt (\(path.lastPathComponent)): \(error.localizedDescription)")
return MCSConfig()
} catch {
output?.warn("Could not read config file: \(error.localizedDescription)")
return MCSConfig()
}Note: YAMLDecoder may throw Yams.YamlError for some malformed inputs, so the "corrupt" catch may need to cover both DecodingError and Yams-specific errors.
|
|
||
| #expect(throws: (any Error).self) { | ||
| try YAMLFile.load(Sample.self, from: path) | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a test for YAML that is syntactically valid but doesn't match the target Decodable type (e.g., "totally: different\nschema: true\n" decoded as Sample). This exercises the Codable-mismatch error path — the most common real-world corruption scenario (user edits a YAML file and introduces a typo in a key name). The current test covers the YAML parser error path but not this distinct decode failure mode.
| /// Throws on read or decode errors so callers can decide how to handle them. | ||
| static func load<T: Decodable>(_ type: T.Type, from path: URL) throws -> T? { | ||
| let fm = FileManager.default | ||
| guard fm.fileExists(atPath: path.path) else { return nil } |
There was a problem hiding this comment.
Suggestion (minor): There's a TOCTOU window between fileExists (line 11) and String(contentsOf:) (line 12). Benign for a CLI tool, but could be eliminated by catching CocoaError.fileReadNoSuchFile from the read call and returning nil, removing the fileExists check. Not blocking — the current approach matches the rest of the codebase.
|
❌ Fixer run - workflow run Status: failure Commits:
|
- Pattern-match on DecodingError and YamlError to report "corrupt" - Fall through to generic catch for I/O errors with "Could not read" message - Re-add Yams import needed for YamlError type reference
- Exercises the Codable-mismatch error path (valid YAML, wrong type) - Covers the most common real-world corruption scenario (wrong keys)
|
🔧 Fixer run - workflow run Status: success Commits:
|
1 participant
Summary
YAMLFilehelper (load/save) that encapsulates the repeated YAML persistence boilerplate (file-exists guard → read string → trim check → decode, and mkdir → encode → write)MCSConfig,PackRegistryFile,ProjectIndex, andLockfile.loadto delegate to the shared helper, each preserving its own error-handling policyLockfile.saveis intentionally unchanged — it prepends a comment header that doesn't fit the generic helperNotes
MCSConfig.loadwraps the helper indo/catchto preserve its swallow-errors behaviorLockfile.savestill usesYAMLEncoderdirectly (retainsimport Yams); the other 3 files drop the importTest plan
YAMLFileTestscovering: missing file, empty file, whitespace-only file, valid decode, corrupt YAML, save with mkdir, round-tripMCSConfigTests,PackRegistryFileTests,ProjectIndexTests,LockfileTestsall pass (68 tests total)Closes #274