Skip to content

Fix issue 742: append CultureInfo.InvariantCulture to QuickFiler date/time rendering - #892

Merged
drmoisan merged 6 commits into
mainfrom
bug/quickfiler-date-time-format-missing-invariant-culture-742
Sep 14, 2026
Merged

drmoisan merged 6 commits into
mainfrom
bug/quickfiler-date-time-format-missing-invariant-culture-742

Conversation

@drmoisan

@drmoisan drmoisan commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Suggested title

Fix issue 742: append CultureInfo.InvariantCulture to QuickFiler date/time rendering

Summary

  • Five QuickFiler controller files render DateTime values through custom format strings (MM/dd/yyyy, HH:mm, HH:mm:ss) whose / and : separators are culture-dependent custom specifiers, not literal characters; under a culture such as it-IT (TimeSeparator = .), now.ToString("HH:mm") renders 13.05 instead of 13:05.
  • All thirteen affected call sites across the five files now pass CultureInfo.InvariantCulture as an explicit second argument (or, for the one interpolated site, are rewritten as explicit two-argument ToString calls), matching the convention already used by adjacent numeric-field formatting in the same methods.
  • Adds using System.Globalization; to the three UI-facing files that previously lacked it.
  • Adds a new regression test file, QuickFiler.Test/Controllers/QuickFilerInvariantCultureIssue742Tests.cs, with five tests (one per production file) that force a sentinel, non-//: culture and assert production output still contains / and :.
  • Repairs two self-referential test oracles in QfcHomeControllerMetricsTests.cs that built their expected value the same uncultured way production code did, which would have made them pass even with the bug present.
  • This is a distinct defect from issue Bug: quickfiler-session-metrics-twelve-hour-time-format #645 (twelve-hour time format); it was discovered during research for that issue.

Why

Five QuickFiler controllers build CSV/log/diagnostic lines and a UI summary string from DateTime.ToString(format) calls that omit CultureInfo.InvariantCulture, while adjacent numeric-field formatting in the same methods already supplies it. : and / in a .NET custom format string are the TimeSeparator and DateSeparator custom specifiers, resolved through the ambient DateTimeFormatInfo, not literal characters — so output silently changes shape under a non-en-US regional configuration. Two of the five affected classes (QfcCollectionController, EfcItemController) carry [ExcludeFromCodeCoverage], so the discovery-count and residual-sweep checks in the plan's evidence, plus the five new tests, are this change's primary correctness signal for those two files rather than a coverage percentage.

What Changed

Core fix (5 production files, 13 call sites):

  • QuickFiler/Controllers/QfcHomeController.Metrics.cs — rewrote the one interpolated site ($"{now:MM/dd/yyyy},{now:HH:mm},") as two explicit ToString(..., CultureInfo.InvariantCulture) calls; added the culture argument to two more sites.
  • QuickFiler/Controllers/EfcHomeController.Metrics.cs — added the culture argument to four sites.
  • QuickFiler/Controllers/QfcItemController.ViewerSetup.cs — added using System.Globalization;; added the culture argument to two sites in GetItemSummary().
  • QuickFiler/Controllers/QfcCollectionController.cs — added using System.Globalization;; added the culture argument to five sites across TryGetMoveReadiness, ToggleExpansionStyle, and GetMoveDiagnostics.
  • QuickFiler/Controllers/EfcItemController.cs — added using System.Globalization;; added the culture argument to two sites (SentDate/SentTime property getters).

Tests:

  • QuickFiler.Test/Controllers/QuickFilerInvariantCultureIssue742Tests.cs (new, 461 lines) — five [TestMethod] tests, one per production file, using an explicit sentinel-culture try/finally swap pattern already established elsewhere in the test suite.
  • QuickFiler.Test/Controllers/QfcHomeControllerMetricsTests.cs — rewrote two expected-value constructions to use CultureInfo.InvariantCulture so the oracle can no longer pass by accident.
  • QuickFiler.Test/QuickFiler.Test.csproj — one new <Compile Include> entry for the new test file.

Docs/evidence: feature folder (docs/features/active/2026-09-02-quickfiler-date-time-format-missing-invariant-culture-742/) — issue, spec, user-story, research, atomic plan (with a coordinator amendment re-deriving two baseline figures after a merge), and Markdown evidence artifacts for every baseline and final QA gate. No raw .trx or Cobertura XML is committed, per the issue #671 evidence-hygiene decision.

Architecture / How It Fits Together

No structural or API change. Each edit is a same-shape ToString call gaining an explicit IFormatProvider argument, or (one site) an interpolated expression rewritten to the equivalent explicit-call form. No signature, interface, or DI wiring changes.

Verification

Completed (see evidence artifacts under the feature folder's evidence/ tree):

  • dotnet tool run csharpier format . / check . — clean, exit 0 (1635 files checked).
  • msbuild TaskMaster.sln /t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true — 0 Error(s), exit 0.
  • msbuild TaskMaster.sln /t:Rebuild ... /p:TreatWarningsAsErrors=true (no /p:Nullable=enable) — 0 Error(s), exit 0.
  • QuickFiler.Test suite via the repository's coverage runner (scripts/vscode/Invoke-MSTestWithCoverage.ps1 -SearchRoot QuickFiler.Test): 1434 of 1434 tests passed, 0 failed (baseline before this change: 1429/1429 passed; the +5 is exactly the new regression tests). No regression in class-level coverage on the two non-excluded, non-100%-covered classes (QfcHomeController 0.75969→0.766917; QfcItemController.ViewerSetup 0.906103→0.910798); EfcHomeController stayed at 1.0.
  • Diff scope confined to exactly the 8 expected QuickFiler/QuickFiler.Test paths (verified via git status --porcelain).

Known gap — not verified as passing, reported rather than worked around: the coverage runner's own exit code for this scoped run is 1, not 0. This is not a test failure — the trx shows 1434/1434 passed both before and after this change. The non-zero exit comes from Assert-CoberturaLineCoverageThreshold in scripts/vscode/Invoke-MSTestWithCoverage.Threshold.ps1, which throws when the Cobertura document's document-level line-rate (spanning every instrumented assembly in the solution, not just QuickFiler.Test) is below a hard-coded 80%. A -SearchRoot QuickFiler.Test-scoped run only exercises one assembly, so it measures ~24.3% against that repo-wide denominator and cannot reach 80% regardless of how many of its own tests pass. This exact exit code and percentage were observed identically on the unfixed baseline tree before any production edit, so it is a pre-existing property of the coverage runner's threshold gate under -SearchRoot scoping, unrelated to this change. One plan task (P5-T4) and its mapped acceptance criterion in spec.md remain unchecked as a result, since their literal acceptance text equates the runner's exit code with the test-failure count, which does not hold on this tree. Recommend promoting this as its own follow-up issue against the coverage runner script rather than resolving it inside this PR, since scripts/vscode/ is outside this issue's Write Set.

Recommended: none beyond the above; the toolchain steps above cover the full CLAUDE.md-mandated loop except for the caveat noted.

Backward Compatibility / Migration Notes

None. No public API, file format, or behavior change other than the date/time separator characters now rendering consistently as / and : regardless of host regional settings.

Risks and Mitigations

  • Risk: a caller elsewhere in the codebase depends on the previous culture-dependent rendering. Mitigation: the diff-scope evidence confirms only the 5 named production files and their direct test coverage changed; no downstream parser of these strings was identified in the affected methods' immediate callers.
  • Risk: CSharpier reflows the multi-argument ToString calls differently than predicted. Mitigation: the plan's acceptance criteria were deliberately written as "count changes without a fixed target count" wherever formatting could vary, and the final csharpier pass is clean with no further diff.

Review Guide

Suggested order:

  1. QuickFiler/Controllers/QfcHomeController.Metrics.cs — the one interpolated-to-explicit-call rewrite (most structurally different edit).
  2. The remaining four production files — mechanical , CultureInfo.InvariantCulture argument additions plus a using directive each.
  3. QuickFiler.Test/Controllers/QuickFilerInvariantCultureIssue742Tests.cs — the new regression tests.
  4. QuickFiler.Test/Controllers/QfcHomeControllerMetricsTests.cs — the two oracle repairs.
  5. QuickFiler.Test/QuickFiler.Test.csproj — the one-line ItemGroup addition.

Nothing in this change is a large mechanical diff; each production file is a small, localized edit.

Follow-ups

  • File a follow-up issue against scripts/vscode/Invoke-MSTestWithCoverage.ps1 / Invoke-MSTestWithCoverage.Threshold.ps1: a -SearchRoot-scoped invocation always fails its post-run exit code via the hard-coded 80% document-level threshold, regardless of the scoped assembly's own test results, which makes the runner's exit code unusable as a pass/fail signal for any future scoped verification task.

GitHub Auto-close

None. GitHub CLI was unavailable in this environment (readiness not verified), and one plan task/acceptance criterion tied to this issue remains open for the reason described above, so no closing keyword is asserted here.

Acceptance criteria: merging at 35 of 36, with AC16 left unchecked

This change merges with 16 of 17 acceptance criteria checked. AC16 is left deliberately unchecked
and the gap is disclosed here rather than resolved, under a maintainer ruling.

AC16's substantive requirement is met. The scoped QuickFiler.Test run reports zero failures:
1434 of 1434 passing, against a 1429 baseline, where the additional five are exactly the regression
tests this change adds.

Its mapped task's literal clause cannot be satisfied in principle.
Assert-CoberturaLineCoverageThreshold in scripts/vscode/Invoke-MSTestWithCoverage.Threshold.ps1
throws unless the document-level Cobertura line-rate clears a hard-coded 80 percent. That
document-level figure spans every instrumented assembly, roughly 61.9 thousand lines, while the
-SearchRoot QuickFiler.Test scoping this plan requires covers about 24.3 percent of that
denominator. The scoping is not optional: it is what avoids the known UtilitiesCS.Test
shell-icon hang. The runner's exit code therefore cannot be 0 for any scoped invocation,
independent of whether any test fails.

This change did not cause it. The same failure was confirmed identically red at task P0-T8, on
the unfixed tree, before any production edit was made.

The criterion defect is tracked separately as issue #891,
coverage-runner-searchroot-threshold-false-fail. Neither the plan nor the coverage-runner script
was modified to work around the condition; both sit outside this issue's declared Write Set.

This follows the precedent already set in this run for a criterion whose literal clause is
unsatisfiable while the property it is actually asserting is true and verified.

@drmoisan
drmoisan merged commit 03d2ece into main Sep 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant