Record CSV output invariantly: ISO 8601 timestamps, full-precision values, real array contents

[BrettKinny]

Summary

Fixes the culture-sensitive / lossy CSV recording output from the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, item 6, plus the related filename-calendar low).

Timestamps — invariant ISO 8601:

• Both WriteRecord timestamp formats (snapshot timestamp and the DateTime.Now fallback) now pass CultureInfo.InvariantCulture. Previously the : custom-format placeholder was replaced by the culture's time separator (fi-FI wrote 14.30.00) and the culture's default calendar applied (th-TH wrote Buddhist year 2569), breaking the documented ISO 8601 contract.
• Default recording filenames (CsvRecordingManager) and connection identifiers (ConnectionIdentifier.Generate) get the same fix — no more Buddhist/Hijri years in default filenames.

Values — full precision, invariant:

• New MonitoredNode.RawValue: an invariant-culture, full-precision string set at the same points the display Value is set (initial read, change notifications, pending/stale markers). The CSV snapshot now records RawValue; the UI display string ("F2", current culture) is untouched.
• New SubscriptionManager.FormatRawValue: float/double via shortest round-trippable ToString(InvariantCulture), DateTime via "O", other IFormattable via invariant culture. Previously every recorded float/double was truncated to 2 decimals (permanent precision loss) and wrote 42,12 under European locales.
• Arrays now record their actual elements, semicolon-joined (e.g. 1;2;3;4;5, including byte[]), instead of "[5 items]". Documented in the README recording bullet.
• The RecordSnapshot immutability/thread-safety design is unchanged: strings captured synchronously at enqueue time on the notification thread.

Tests: 39 new tests — ISO 8601 under fi-FI/th-TH hostile cultures, full-precision round-trips, dot decimals under fi-FI/de-DE/th-TH, array serialization, Gregorian filenames under th-TH, snapshot immutability. Temporarily reverting only CsvRecordingManager.cs makes 6 of them fail, confirming they catch the original defects. (One subtlety: xUnit's Assert.DoesNotContain(string) is itself culture-sensitive under th-TH collation, so an exact Assert.Equal is used instead.)

Test plan

• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• dotnet test -c Release — 639/639 passing (full suite incl. integration)

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, item 6).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[claude]

Code Review: CSV Invariant Output Fix

Summary: This PR correctly fixes culture-sensitive bugs in CSV recording — timestamp formatting that broke under fi-FI/th-TH locales, float/double values truncated to 2 decimal places in the display format, and arrays serialized as [N items]. The approach is sound and the test coverage is thorough.

---

What's done well

• Root-cause fix, not a workaround. Every call-site that formats data for export now passes CultureInfo.InvariantCulture explicitly. No fragile ambient-culture hacks.
• Clean separation between display and export. FormatValue (F2, current culture, human-readable) vs FormatRawValue (round-trip, invariant, machine-readable) is the right design, and the XML doc on each makes the contract explicit.
• The "O" round-trip format for DateTime is the right choice — it encodes DateTimeKind and produces unambiguous ISO 8601 with timezone offset.
• Fallback string.IsNullOrEmpty(item.RawValue) ? item.Value : item.RawValue is defensive without being fragile. Handles the = string.Empty default safely.
• Test design is excellent. 39 tests, hostile cultures (fi-FI, de-DE, th-TH), round-trip assertions, snapshot immutability, the Assert.DoesNotContain collation subtlety under th-TH properly handled with Assert.Equal. The note about temporarily reverting the fix to verify 6 tests fail is good practice.

---

Issues worth addressing

1. Semicolons within string array elements are not escaped (potential data loss)

FormatRawValue(new[] { "a;b", "c" }) → "a;b;c", which is indistinguishable from FormatRawValue(new[] { "a", "b", "c" }). Anyone importing the CSV and splitting on ; will silently misparse multi-element string arrays.

OPC UA String[] values aren't common, but they're valid. Consider doubling or escaping internal semicolons, or documenting the limitation prominently.

// Option A: escape ';' within elements
parts.Add(FormatRawValue(element).Replace(";", "\;"));

// Option B: document the known limitation in the XML doc comment

2. WithCulture helper is duplicated across two test files

SubscriptionManagerTests.cs and CsvRecordingManagerTests.cs both define an identical private static void WithCulture(string, Action). A shared TestHelpers class in Tests/Opcilloscope.Tests/Infrastructure/ would keep them in sync and remove the duplication.

3. Minor: XML doc on MonitoredNode.RawValue describes implementation, not contract

The current doc comment explains how the value is set (references SubscriptionManager.FormatRawValue, call sites). Model properties typically document the what, leaving the how to the code that sets them. A shorter doc is less likely to drift:

/// <summary>
/// Full-precision, culture-invariant value for data export (CSV).
/// Set at the same points as <see cref="Value"/>; see
/// <c>SubscriptionManager.FormatRawValue</c> for formatting rules.
/// </summary>

---

Minor observations (no action required)

• bool falls through to value.ToString() (not IFormattable in C#), producing "True"/"False". This is correct and the test confirms it.
• (pending) / (reconnecting...) in RawValue — intentional transient markers, will appear in CSV rows written during those windows. This is consistent with how Value works and is the right call.
• GenerateDefaultRecordingFilename_UsesGregorianCalendar_UnderThaiCulture uses DateTime.Now.Year in both the assertion and the SUT, so it's not testing a fixed timestamp. It would fail at year boundary (UTC midnight Dec 31 / Jan 1) only if the year increments between the two calls — an astronomically unlikely race, but worth noting if this ever appears flaky in CI.

---

Verdict

Approve with the semicolon-escaping issue flagged as the only non-trivial concern. The core bug fixes are correct, the test coverage is solid, and the separation of FormatValue / FormatRawValue is a clean design that will prevent this class of bug from recurring. The semicolon issue won't affect numeric arrays (the common case in OPC UA), but it's a correctness gap for string arrays that should be resolved before this merges.

[claude]

Code Review

Note: This PR is already merged — feedback is offered for the record and future reference.

Overview

This PR fixes a real and well-scoped bug: DateTime.ToString(format) without an explicit culture uses the thread's current culture, which can emit . as the time separator under fi-FI and Buddhist calendar years under th-TH — both breaking the documented ISO 8601 contract. The fix is correct and comprehensive: every timestamp format call gets CultureInfo.InvariantCulture, display and export values are cleanly separated via RawValue, and 39 new tests exercise hostile cultures end-to-end.

---

What Works Well

Correct diagnosis and fix. Pinning CultureInfo.InvariantCulture on every DateTime.ToString call is exactly right. The bug is subtle (only surfaces under non-English system locales) and the fix is minimal.

Clean display/export separation. The RawValue / Value split on MonitoredNode is a good design: the UI display string stays culture-aware and human-readable ("F2", current culture), while the CSV export path gets a lossless, invariant representation. The fallback (string.IsNullOrEmpty(RawValue) ? Value : RawValue) is sensible for nodes that haven't received a value yet.

Array serialization is well thought-out. Using ; as the element separator (avoiding the CSV comma issue) and escaping \ before ; (correct escape-the-escape-character-first order) is correct. The resulting strings never need CSV comma-escaping for the element separator itself.

Impressive th-TH collation catch. The comment explaining why Assert.DoesNotContain(string) is replaced with Assert.Equal under th-TH (punctuation is treated as ignorable by that collation, so substring checks can produce false negatives) is a genuinely subtle observation and a good example of a comment whose why is non-obvious.

Test quality is high. Round-trip precision tests (double.Parse(FormatRawValue(x)) == x), snapshot immutability test, and the six-test revert validation described in the PR body are all solid.

---

Issues and Suggestions

Minor

WithCulture helper is duplicated. The same private helper appears verbatim in both CsvRecordingManagerTests.cs and SubscriptionManagerTests.cs. It could live in a shared TestHelpers or CultureHelper class in Tests/Opcilloscope.Tests/Infrastructure/. Not a bug, but worth consolidating before the next test that needs it.

Empty array serializes to empty string. FormatRawValue(Array.Empty<int>()) returns "", which is indistinguishable in the CSV from a missing/null value. A consumer parsing the CSV cannot tell whether an empty field means "no value" or "zero-element array". The test documents this behavior as intentional, but it's worth a comment in FormatRawValue explaining the deliberate choice, or a future ticket to consider a distinguishing token (e.g. "[]").

"null" string for null values. Both FormatValue and FormatRawValue return the string "null" when the value is null. A CSV consumer cannot distinguish this from an OPC UA String node that happens to contain the text "null". This pre-exists the PR, but since RawValue is new export infrastructure it's a good time to consider escaping or quoting.

bool falls through to value.ToString(). bool does not implement IFormattable, so it bypasses the IFormattable branch and hits the final value.ToString() ?? "null". bool.ToString() in .NET always returns "True" / "False" regardless of culture, so this is safe — but the test comment says "True/False" without explaining why it's safe (i.e. that bool has no culture-sensitive overload). A brief inline note would help future readers.

Nit

The doc comments on FormatValue and FormatRawValue are helpful given the explicit "never use this for data export" warning, but they're longer than the one-short-line convention in CLAUDE.md. Consider trimming to the essential caution rather than restating the implementation.

---

Summary

Correct fix, correct approach, good test coverage. The two substantive observations worth tracking are the empty-array ambiguity and the null-string ambiguity — both edge cases that don't affect current usage but could trip up future CSV consumers. The WithCulture duplication is the only clear cleanup target. Overall this is a high-quality, well-motivated change.