Skip to content

refactor(sleep): one stage-key canonicalisation, not two kept in parity by comment#829

Merged
ryanbr merged 4 commits into
mainfrom
refactor/dedupe-stage-canonicalisation
Jul 26, 2026
Merged

refactor(sleep): one stage-key canonicalisation, not two kept in parity by comment#829
ryanbr merged 4 commits into
mainfrom
refactor/dedupe-stage-canonicalisation

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Follow-up to #733, found reviewing it.

Two implementations of one rule, and the code already knew. stageColorFor did its own trim().lowercase() and folded "awake", "wake" in a when branch, while canonicalStage() in SleepStageTimelineLogic.kt does exactly the same — carrying the comment "(stageColorFor parity)". A drift risk documented instead of removed.

// before
private fun stageColorFor(name: String): Color = when (name.trim().lowercase()) {
    …
    "awake", "wake" -> Palette.sleepAwake

// after
private fun stageColorFor(name: String): Color = when (canonicalStage(name)) {
    …
    "awake" -> Palette.sleepAwake

Both files are package com.noop.ui and canonicalStage is internal, so no plumbing. Adding a stage alias is now a one-place edit — before, a new alias in canonicalStage would silently fail to pick up a colour, which is the quiet-wrong-answer failure mode rather than a visible one.

Behaviour-identical, verified across every input the old branch accepted: "deep", " REM ", "Light", "awake", "WAKE", "wake", whitespace variants, empty string and unknown all resolve to the same tint before and after, fallback included.

#733 didn't create this — it made it visible by moving the two halves into adjacent files, which is a good argument for that refactor rather than against it.

Android only, no schema, no scores, no strings; i18n clean. Not compiled — android.yml is disabled — though this is a two-line change to a when subject within one package.

ryanbr added 3 commits July 25, 2026 23:58
…ty by comment

stageColorFor did its own trim().lowercase() and folded "wake"/"awake" in a when
branch, duplicating canonicalStage() in SleepStageTimelineLogic. The duplication was
known — canonicalStage carried the comment "(stageColorFor parity)" — which is a
drift risk documented rather than removed.

#733 did not create this; it made it visible by moving the two halves into adjacent
files.

stageColorFor now keys off canonicalStage. Both files are package com.noop.ui and
canonicalStage is internal, so no plumbing was needed. Adding a stage alias is now a
one-place edit and the tint follows; before, a new alias in canonicalStage would
silently not colour.

Behaviour-identical, checked across every input the old branch accepted — "deep",
" REM ", "Light", "awake", "WAKE", "wake", whitespace variants, empty and unknown
all resolve to the same tint before and after, fallback included.

Android only. No schema, no scores, no strings — the i18n audit is clean and the
moved code uses uiString(R.string.…) lookups throughout.
[stageColorFor] would not resolve from SleepStageTimelineLogic: the target is
private in SleepStageBreakdownUi.kt, so it is not in scope there and Dokka would
warn on it. The reverse link is fine — [canonicalStage] is internal in the same
package, which does resolve.

Replaced with a code span plus the filename. A pointer that cannot be followed is
worse than one that reads as prose, and the asymmetry is easy to miss because the
two links look identical.
The phrase described a hand-maintained invariant. It isn't one any more — the tint
derives from the same canonicalStage call the spans do, so this test now covers the
colour path as a side effect. Say that instead, and note why there's no direct
assertion: stageColorFor is private.
@ryanbr

ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Re-review found two things in my own change, both now pushed.

1. A KDoc link that could not resolve. [stageColorFor] from SleepStageTimelineLogic.kt points at a private function in another file — not in scope there, so Dokka would warn. The reverse link is fine: [canonicalStage] is internal in the same package. The asymmetry is easy to miss because the two links look identical. Now a code span plus the filename.

2. The last (stageColorFor parity) note, in StageRowSpansTest.kt:33. That phrase described a hand-maintained invariant, which is exactly what this PR removes — leaving it means grepping it turns up a stale hint. The test now covers the colour path as a side effect of covering the spans, so it says that instead.

No behaviour change in either. CI green, three commits.

I added a doc block above the existing one instead of editing it, leaving two
consecutive /** */ before the same declaration. Kotlin takes only the last as KDoc;
the first silently becomes a dangling comment, so the "case-insensitive" note was no
longer attached to anything.

Merged, keeping the original wording and the fallback behaviour, which the old block
did not state. Duplicate documentation in a PR about removing duplicate logic.
@ryanbr

ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Third pass, one more of my own: I had added a doc block above the existing one on stageColorFor instead of editing it, leaving two consecutive /** */ on the same declaration. Kotlin takes only the last as KDoc, so the original case-insensitive note silently became a dangling comment. Duplicate documentation, in a PR about removing duplicate logic.

Merged into one block, keeping the original wording and adding the unknown-stage fallback, which neither block stated.

CI green, MERGEABLE/CLEAN, +13/-5.

Out of scope here, but the same defect exists in two other places and I put both there: PuffinExperiment.kt:118 (harmless — near-identical text) and HrvAnalyzer.kt:342, where the detached block is the one reading "Byte-parity twin of Swift classifyCoverage". That is a parity-contract note that is no longer attached to the function it documents. Happy to fix both in a separate one-liner PR.

@ryanbr
ryanbr merged commit 10f8399 into main Jul 26, 2026
1 check passed
@ryanbr
ryanbr deleted the refactor/dedupe-stage-canonicalisation branch July 26, 2026 07:12
ryanbr added a commit that referenced this pull request Jul 27, 2026
Three things found re-reviewing my own change.

The app_name exemption was defined twice — once inside android_strings_xml_gaps and
again inline in the new discovered-locale check. That is the same two-definitions-of-
one-rule pattern #829 removed this morning, and it would have let the hard-gated path
and the discovered path disagree the first time anyone added a second exempt key.
Hoisted to ANDROID_EXEMPT_KEYS, used by both.

The new check re-loaded every catalog the loop above had just parsed — a second full
parse of a 3255-string file for no reason. The extra-locale counts are now collected
inside that existing loop and gated afterwards.

--update-baseline rewrites the JSON baseline only, so its help now says it does not
touch the locale allowance. That file is lowered by hand on purpose: shrinking it
should be a deliberate act, not a side effect of regenerating something else.

No behaviour change — all four gate cases re-verified after the refactor.
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