From 18fc52383c5c30a24b65c3f061b14d76b01b7209 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 19 Aug 2026 16:32:06 -0700 Subject: [PATCH] clean up error output --- src/cache/store.zig | 10 ++++-- src/data/imported_values.zig | 65 +++++++++++++++++++++++++++++++++--- src/history.zig | 40 ++++++++++++++++++++++ 3 files changed, 108 insertions(+), 7 deletions(-) diff --git a/src/cache/store.zig b/src/cache/store.zig index d0677dc..f20d655 100644 --- a/src/cache/store.zig +++ b/src/cache/store.zig @@ -1,4 +1,5 @@ const std = @import("std"); +const builtin = @import("builtin"); const log = std.log.scoped(.cache); const srf = @import("srf"); const srf_opts = @import("../srf_opts.zig"); @@ -2190,7 +2191,10 @@ pub fn deserializePortfolioDiag( var lot = fields.to(Lot, srf_opts.user_edited) catch |err| { if (diags) |d| { try appendParseDiag(allocator, d, data, line, @errorName(err)); - } else { + } else if (!builtin.is_test) { + // Quiet under `zig build test`: fixtures feed malformed + // records on purpose to pin the skip, and the warn spam + // pollutes every run's output. std.log.warn("portfolio: could not parse record at line {d}: {s}", .{ line, @errorName(err) }); } skipped += 1; @@ -2214,7 +2218,7 @@ pub fn deserializePortfolioDiag( else => { if (diags) |d| { try appendParseDiag(allocator, d, data, line, "no symbol"); - } else { + } else if (!builtin.is_test) { std.log.warn("portfolio: record at line {d} has no symbol, skipping", .{line}); } if (lot.note) |n| allocator.free(n); @@ -2234,7 +2238,7 @@ pub fn deserializePortfolioDiag( // Only log the rollup when nobody is collecting: a caller with // `diags` reports the count itself, in band, and would otherwise say // it twice. - if (skipped > 0 and diags == null) { + if (skipped > 0 and diags == null and !builtin.is_test) { std.log.warn("portfolio: {d} record(s) could not be parsed and were skipped", .{skipped}); } diff --git a/src/data/imported_values.zig b/src/data/imported_values.zig index 17d7da9..62f0326 100644 --- a/src/data/imported_values.zig +++ b/src/data/imported_values.zig @@ -34,10 +34,13 @@ //! "model said you're already there", or absent. const std = @import("std"); +const builtin = @import("builtin"); const srf = @import("srf"); const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); +const log = std.log.scoped(.imported_values); + // ── Types ──────────────────────────────────────────────────── /// Projection of "when can the user retire," as captured at a @@ -179,10 +182,25 @@ pub fn parseImportedValues( while (it.next() catch return error.InvalidSrf) |fields| { // Aborts rather than skipping: this is a small curated series, // so one unparseable row means the transcription is suspect and - // a partial series would be worse than none. The error passes - // through instead of collapsing to `InvalidSrf`, so the caller - // can report WHY it failed rather than just that it did. - const point = try fields.to(HistoryPoint, srf_opts.user_edited); + // a partial series would be worse than none. + // + // Collapses to `InvalidSrf` on purpose, and logs the real error + // instead of returning it. `history.resolveAsOfDate` enumerates + // this function's parse errors to degrade gracefully to + // "no data at or before" - and srf's coercion errors are an + // open-ended set, so propagating them raw would silently fall + // through that switch and surface an unexplained error instead + // of the actionable "run `zfin snapshot` or populate + // imported_values.srf" message. The log carries the cause; the + // error set stays enumerable. + const point = fields.to(HistoryPoint, srf_opts.user_edited) catch |err| { + // Quiet under `zig build test`, where a fixture feeds a + // malformed row on purpose to pin the collapse above. + if (!builtin.is_test) { + log.warn("imported_values.srf: record {d}: {s}", .{ points.items.len + 1, @errorName(err) }); + } + return error.InvalidSrf; + }; try points.append(allocator, point); } @@ -289,6 +307,45 @@ test "parseImportedValues: missing optional fields" { try std.testing.expectEqual(@as(?ProjectedRetirement, null), iv.points[0].projected_retirement); } +test "parseImportedValues: an unparseable record collapses to InvalidSrf" { + // REGRESSION GUARD. `history.resolveAsOfDate` enumerates this + // function's parse errors to degrade gracefully to + // `NoDataAtOrBefore`, which is what produces the actionable "run + // `zfin snapshot` or populate imported_values.srf" message. srf's + // coercion errors are an open-ended set, so letting one through raw + // fell past that switch and surfaced an unexplained error instead. + // + // `liquid` is declared numeric; a string separator makes it a + // coercion failure that `strings_to_numbers` cannot rescue, since + // "abc" is not a number either. + const data = + \\#!srfv1 + \\date::2014-07-03,liquid::abc + \\ + ; + try std.testing.expectError( + error.InvalidSrf, + parseImportedValues(std.testing.allocator, data), + ); +} + +test "parseImportedValues: a missing required field also collapses to InvalidSrf" { + // A second, structurally different srf error. `date` and `liquid` + // have no defaults, so omitting one fails coercion via a different + // path than a bad value does - and it must collapse identically, + // because the point is that the caller's error set stays enumerable + // no matter which way coercion fails. + const data = + \\#!srfv1 + \\liquid:num:1280036.42 + \\ + ; + try std.testing.expectError( + error.InvalidSrf, + parseImportedValues(std.testing.allocator, data), + ); +} + test "parseImportedValues: empty file (header only)" { const data = "#!srfv1\n"; var iv = try parseImportedValues(std.testing.allocator, data); diff --git a/src/history.zig b/src/history.zig index 2aaa99e..19604c6 100644 --- a/src/history.zig +++ b/src/history.zig @@ -1433,6 +1433,46 @@ test "resolveAsOfDate: imported-only falls back to imported_values" { try testing.expectEqual(@as(f64, 1_510_000), r.liquid); } +test "resolveAsOfDate: a malformed imported_values.srf degrades to NoDataAtOrBefore" { + // THE REGRESSION THIS GUARDS. The switch below enumerates + // `parseImportedValues`' parse errors to degrade gracefully, which is + // what lets `resolveAsOfOrExplain` print the actionable "run + // `zfin snapshot` or populate imported_values.srf" message. + // + // srf's coercion errors are an open-ended set, so when + // `parseImportedValues` was briefly changed to propagate them raw + // they fell straight through this switch and the user got an + // unexplained `IntNotNumberType` instead. The collapse to + // `InvalidSrf` is load-bearing, not laziness - see the comment in + // `parseImportedValues`. + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + // `liquid` is declared numeric; a string separator with a + // non-numeric value is a coercion failure `strings_to_numbers` + // cannot rescue. + try tmp.dir.writeFile(io, .{ .sub_path = "imported_values.srf", .data = + \\#!srfv1 + \\date::2016-01-03,liquid::abc + \\ + }); + + const hist_dir = try tmp.dir.realPathFileAlloc(io, ".", testing.allocator); + defer testing.allocator.free(hist_dir); + + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + // No snapshots either, so the imported path is the only candidate - + // and an unusable file must read as "no data", not as a raw parse + // error escaping to the command. + try testing.expectError( + error.NoDataAtOrBefore, + resolveAsOfDate(io, arena.allocator(), hist_dir, Date.fromYmd(2016, 6, 1)), + ); +} + test "resolveAsOfDate: snapshot wins over imported when both present" { const io = std.testing.io; var tmp = std.testing.tmpDir(.{});