Fix issue 742: append CultureInfo.InvariantCulture to QuickFiler date/time rendering - #892
Merged
drmoisan merged 6 commits intoSep 14, 2026
Conversation
…story, and preflight-cleared atomic plan
…ime-format-missing-invariant-culture-742
…w coverage read location
…/time rendering Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested title
Fix issue 742: append CultureInfo.InvariantCulture to QuickFiler date/time rendering
Summary
QuickFilercontroller files renderDateTimevalues 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 asit-IT(TimeSeparator=.),now.ToString("HH:mm")renders13.05instead of13:05.CultureInfo.InvariantCultureas an explicit second argument (or, for the one interpolated site, are rewritten as explicit two-argumentToStringcalls), matching the convention already used by adjacent numeric-field formatting in the same methods.using System.Globalization;to the three UI-facing files that previously lacked it.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:.QfcHomeControllerMetricsTests.csthat built their expected value the same uncultured way production code did, which would have made them pass even with the bug present.Why
Five
QuickFilercontrollers build CSV/log/diagnostic lines and a UI summary string fromDateTime.ToString(format)calls that omitCultureInfo.InvariantCulture, while adjacent numeric-field formatting in the same methods already supplies it.:and/in a .NET custom format string are theTimeSeparatorandDateSeparatorcustom specifiers, resolved through the ambientDateTimeFormatInfo, not literal characters — so output silently changes shape under a non-en-USregional 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 explicitToString(..., 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— addedusing System.Globalization;; added the culture argument to two sites inGetItemSummary().QuickFiler/Controllers/QfcCollectionController.cs— addedusing System.Globalization;; added the culture argument to five sites acrossTryGetMoveReadiness,ToggleExpansionStyle, andGetMoveDiagnostics.QuickFiler/Controllers/EfcItemController.cs— addedusing System.Globalization;; added the culture argument to two sites (SentDate/SentTimeproperty 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 useCultureInfo.InvariantCultureso 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.trxor 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
ToStringcall gaining an explicitIFormatProviderargument, 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.Testsuite 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 (QfcHomeController0.75969→0.766917;QfcItemController.ViewerSetup0.906103→0.910798);EfcHomeControllerstayed at 1.0.QuickFiler/QuickFiler.Testpaths (verified viagit 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, not0. This is not a test failure — the trx shows 1434/1434 passed both before and after this change. The non-zero exit comes fromAssert-CoberturaLineCoverageThresholdinscripts/vscode/Invoke-MSTestWithCoverage.Threshold.ps1, which throws when the Cobertura document's document-levelline-rate(spanning every instrumented assembly in the solution, not justQuickFiler.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-SearchRootscoping, unrelated to this change. One plan task (P5-T4) and its mapped acceptance criterion inspec.mdremain 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, sincescripts/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
ToStringcalls 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:
QuickFiler/Controllers/QfcHomeController.Metrics.cs— the one interpolated-to-explicit-call rewrite (most structurally different edit)., CultureInfo.InvariantCultureargument additions plus ausingdirective each.QuickFiler.Test/Controllers/QuickFilerInvariantCultureIssue742Tests.cs— the new regression tests.QuickFiler.Test/Controllers/QfcHomeControllerMetricsTests.cs— the two oracle repairs.QuickFiler.Test/QuickFiler.Test.csproj— the one-lineItemGroupaddition.Nothing in this change is a large mechanical diff; each production file is a small, localized edit.
Follow-ups
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.Testrun 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-CoberturaLineCoverageThresholdinscripts/vscode/Invoke-MSTestWithCoverage.Threshold.ps1throws 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.Testscoping this plan requires covers about 24.3 percent of thatdenominator. The scoping is not optional: it is what avoids the known
UtilitiesCS.Testshell-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 scriptwas 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.