Fix reading the display on a SKY - #1168
Draft
joostlek wants to merge 1 commit into
Draft
Conversation
The screensaver modes were modelled from a TIME, which reports time_based and when_dark, and both were required. A SKY reports time_based and screen_off, so reading its display raised MissingField: Field "when_dark" of type DisplayScreensaverWhenDark is missing in DisplayScreensaverModes instance which fails device() outright and so breaks setting up a SKY at all. Every mode is now optional, and screen_off is modelled and selectable through ScreensaverMode. Which modes a device reports depends on the hardware, so this stops being an assumption about any one model. The device fixtures were all TIMEs, which is why this was missed. Adds a SKY fixture, taken from a real device, to the device test and covers the mode difference explicitly. Note that time_based and when_dark are now optional and so need a None check; mashumaro raised on the missing field before, rather than returning None, so nothing that worked before stops working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
joostlek
marked this pull request as draft
September 10, 2026 18:18
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.
Proposed Changes
A regression in #1162, released in v1.4.0. Reading the display on a SKY raises:
That propagates out of
device()anddisplay(), so a SKY cannot be set up at all on v1.4.0.The cause is that I modelled
screensaver.modesfrom a TIME, which reportstime_basedandwhen_dark, and made both fields required. A SKY reportstime_basedandscreen_off:Which modes a device reports depends on the hardware, so every mode is now optional rather than the model asserting any particular set.
screen_offis modelled, and added toScreensaverModeso it can be selected on a SKY.Why it was missed
Every device fixture in the repo is a TIME —
device.jsonanddevice2.jsonare anLM 37X8,device3.jsonis ansa8— so there was no SKY payload to contradict the assumption. This addsdevice_sky.json, taken from a real SKY, to the parametrizedtest_get_device, plus a test covering the mode difference explicitly. The snapshot changes are purely additive: the existing TIME snapshots gain'screen_off': None, with no values altered.Compatibility
time_basedandwhen_darkare now| Noneand so need a None check. This is visible to type checkers, and updating this repo's own tests surfaced 12 such errors, which is a fair preview of what a consumer will see.It is not a behavioural break, though: previously a missing mode raised
MissingFieldrather than yieldingNone, so any code path that worked before still works. Only code that already had to handle the exception now gets aNoneinstead.Verified both ways: a live LaMetric TIME (
sa8, OS 3.2.7) still reads correctly withwhen_darkpopulated andscreen_offasNone, and the SKY payload now parses with the reverse.Related Issues
Regression from #1162.
🤖 Generated with Claude Code