refactor(sleep): one stage-key canonicalisation, not two kept in parity by comment#829
Conversation
…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.
|
Re-review found two things in my own change, both now pushed. 1. A KDoc link that could not resolve. 2. The last 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.
|
Third pass, one more of my own: I had added a doc block above the existing one on 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: |
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.
Follow-up to #733, found reviewing it.
Two implementations of one rule, and the code already knew.
stageColorFordid its owntrim().lowercase()and folded"awake", "wake"in awhenbranch, whilecanonicalStage()inSleepStageTimelineLogic.ktdoes exactly the same — carrying the comment "(stageColorFor parity)". A drift risk documented instead of removed.Both files are
package com.noop.uiandcanonicalStageisinternal, so no plumbing. Adding a stage alias is now a one-place edit — before, a new alias incanonicalStagewould 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.ymlis disabled — though this is a two-line change to awhensubject within one package.