Skip to content

fix(testing): PluginDimmer DON takes an on-level param so it classifies LIGHT#171

Merged
shbatm merged 1 commit into
mainfrom
fix/159-plugin-dimmer-fixture
May 18, 2026
Merged

fix(testing): PluginDimmer DON takes an on-level param so it classifies LIGHT#171
shbatm merged 1 commit into
mainfrom
fix/159-plugin-dimmer-fixture

Conversation

@shbatm

@shbatm shbatm commented May 18, 2026

Copy link
Copy Markdown
Owner

Closes #159

Problem

pyisyox/testing/__init__.py _build_plugin_dimmer_nodedef() is named/exposed as a dimmer (PLUGIN_DIMMER_NODEDEF_ID = "PluginDimmer", docstring "light controllable") but its DON was parameterless:

{"id": "DON", "name": "On"},

Post-#158, a parameterless DON correctly classifies as SWITCH (HA can't drive brightness via a bare DON). The fixture's name/docstring then contradicted its actual classification. The pyisyox suite never asserted its controllable, so it slipped through; it surfaced downstream in hacs-udi-iox's suite.

Fix — resolution (1) from the issue (maintainer-preferred)

Give DON an I_OL on-level param so it genuinely classifies LIGHT (the on-level param is exactly what _detect_controllable's on_takes_level looks for; combined with the fixture's existing BRT/DIM dimmer hints → LIGHT). The fixture's name/docstring are now accurate. Param style matches the fixture's own SETMODE/THRESHOLD params ({"id": "", "editor": "..."}).

Docstring updated to state why the param matters (so it can't be "cleaned up" back to parameterless).

Regression guard

New test_dimmer_plugin_nodedef_classifies_light asserts classify(nd).controllable is ControllablePlatform.LIGHT — the assertion the issue explicitly asked for, so the fixture can't silently drift again.

Verification

  • New test passes; full suite 839 passed — no regressions, confirming nothing in the pyisyox suite relied on the old SWITCH misclassification.
  • ruff / ruff-format / mypy / pylint(10) / codespell / pre-commit green.

Test-infra only; not user-facing. Pairs with #170 for the upcoming beta bump.

🤖 Generated with Claude Code

…es LIGHT

`_build_plugin_dimmer_nodedef`'s `DON` was parameterless, which
post-#158 correctly classifies as SWITCH (HA can't drive brightness
via a bare `DON`). The fixture's name/docstring
(`PLUGIN_DIMMER_NODEDEF_ID`, "light controllable") then contradicted
its actual classification. Resolution (1) from the issue: give `DON`
an `I_OL` on-level param so it genuinely classifies LIGHT, matching
the fixture's purpose (the only LIGHT-plugin fixture). Adds a
regression assertion on `classify().controllable` so it can't drift
again.

Closes #159

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

Claude finished @shbatm's task in 41s —— View job


Code Review — PR #171

  • Gather context and understand the request
  • Read changed files
  • Analyze the fix and regression guard
  • Post review feedback

Summary: This is a targeted, correct fix with a solid regression guard. The change is minimal and well-reasoned.

pyisyox/testing/__init__.py

The fix itself (_build_plugin_dimmer_nodedef, lines 920–924) is exactly right. The classifier at classifier.py:557 computes:

on_takes_level = on_cmd is not None and len(on_cmd.parameters) > 0

Then at classifier.py:323:

is_dimmer = (has_on_level or bool(_LIGHT_DIMMER_HINTS & accept_ids)) and on_takes_level

The fixture already had BRT/DIM (the _LIGHT_DIMMER_HINTS), so the only missing piece was on_takes_level. Adding {"id": "", "editor": "I_OL"} as a parameter to DON flips on_takes_level to True, yielding LIGHT. The param style matches the existing SETMODE/THRESHOLD/INVERT params in the same nodedef.

The docstring update is good engineering hygiene. Embedding the why (parameterless DON → SWITCH, post-#158) directly in the docstring is the right call — it's non-obvious and will prevent a future "cleanup" from reverting the change.

One very minor nit: the docstring is now somewhat long for a single run-on sentence in the opening summary line. Not a blocker at all, just an observation.

tests/test_testing/test_builders.py

The regression test (test_dimmer_plugin_nodedef_classifies_light, lines 415–427) is well-structured:

  • Uses make_profile_with_dimmer_plugin() (the fresh, non-cached path — correctly avoids the shared load_profile() instance).
  • Calls profile.find_nodedef(...) with all three lookup keys — correct lookup shape.
  • Asserts classify(nd).controllable is ControllablePlatform.LIGHT — exactly the assertion that was missing before and that would have caught this at the pyisyox level.
  • The assert nd is not None guard before the classify call avoids a confusing AttributeError if the nodedef lookup ever breaks.

The imports added (ControllablePlatform, classify, PLUGIN_DIMMER_INSTANCE_ID, PLUGIN_DIMMER_NODEDEF_ID) are all needed and appropriately placed alongside the existing imports.

Overall

No correctness issues, no regressions. The fix is minimal and anchored to the exact classifier invariant it's restoring. The test locks in the contract so downstream consumers won't silently break again.

Approved.

@shbatm
shbatm merged commit 9d2542d into main May 18, 2026
5 checks passed
@shbatm
shbatm deleted the fix/159-plugin-dimmer-fixture branch May 18, 2026 21:48
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.

_build_plugin_dimmer_nodedef is misnamed post-#158 (parameterless DON → SWITCH, not LIGHT)

1 participant