Skip to content

Add SwitchbotKeypad device class for classic Keypad#488

Open
italo-lombardi wants to merge 9 commits into
sblibs:masterfrom
italo-lombardi:feature/switchbot-keypad
Open

Add SwitchbotKeypad device class for classic Keypad#488
italo-lombardi wants to merge 9 commits into
sblibs:masterfrom
italo-lombardi:feature/switchbot-keypad

Conversation

@italo-lombardi
Copy link
Copy Markdown

AI-generated PR to support Switchbot Keypad and mainly expose battery levels
https://eu.switch-bot.com/products/switchbot-keypad
The code was not tested, but it would be great to get some help/feedback from you. Thanks

Description:

Summary

The classic SwitchBot Keypad (WoKeypad) is a passive BLE-only device
used to unlock doors paired with SwitchBot Lock. The advertisement
parser (adv_parsers/keypad.py) already parses battery and
attempt_state from BLE advertisements, and SwitchbotModel.KEYPAD
is already defined, but no device class existed.

Changes

  • Add SwitchbotKeypad device class in devices/keypad.py, extending
    SwitchbotDevice (passive-only, no BLE commands)
  • Export SwitchbotKeypad from switchbot/__init__.py
  • Add KEYPAD_INFO test fixture in tests/__init__.py
  • Add tests/test_keypad.py verifying battery and attempt_state
    are correctly parsed from advertisement data

italo-lombardi and others added 3 commits May 6, 2026 11:28
The classic Keypad (WoKeypad) is a passive BLE-only device paired
with SwitchBot Lock. The adv parser already exposes battery and
attempt_state from advertisement data. This adds the device class
and exports it so integrations can reference it explicitly.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/__init__.py 100.00% <100.00%> (ø)
switchbot/devices/keypad.py 100.00% <100.00%> (+100.00%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco bdraco requested a review from Copilot May 14, 2026 20:00
@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 14, 2026

@bluetoothbot review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a minimal SwitchbotKeypad device class that wraps the existing process_wokeypad advertisement parser, exposes it from the package's public API, and adds a fixture plus three tests verifying battery and attempt_state parsing.

Changes:

  • New switchbot/devices/keypad.py defining SwitchbotKeypad (subclass of SwitchbotDevice, no commands).
  • Public export added in switchbot/__init__.py (SwitchbotKeypad).
  • New KEYPAD_INFO AdvTestCase fixture in tests/__init__.py and a new tests/test_keypad.py exercising advertisement parsing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
switchbot/devices/keypad.py Adds the new passive SwitchbotKeypad device class.
switchbot/init.py Imports and re-exports SwitchbotKeypad in the public API.
tests/init.py Adds KEYPAD_INFO advertisement fixture used by the new tests.
tests/test_keypad.py Adds tests verifying parsed battery and attempt_state values, including battery=None override.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bluetoothbot
Copy link
Copy Markdown
Collaborator

bluetoothbot commented May 14, 2026

PR Review — Add SwitchbotKeypad device class for classic Keypad

Small, well-scoped change that fills a real gap: the process_wokeypad parser and SwitchbotModel.KEYPAD enum already existed, but no device class consumed them. Implementation is correctly minimal (passive BLE — no commands), the public export is alphabetized, and codecov reports 100% coverage on the new code. The author addressed the prior automated review by driving tests through parse_advertisement_data with real BLE bytes and asserting on get_battery_percent() rather than stuffing pre-parsed values — that's the right call and the tests now actually exercise the parser path end-to-end. Author flagged that the code wasn't tested against real hardware, but the byte-layout is the same as the existing test_parse_advertisement_data_keypad fixture in test_adv_parser.py:1450, so regressions would surface. Merge-ready; the inline comments are nits.


🟢 Suggestions

1. Repeated setup could be pulled into a fixture or parametrize (`tests/test_keypad.py`, L9-58)

All three tests duplicate the BLE device + advertisement construction. With three short tests it is acceptable as-is, but extracting a small helper or using @pytest.fixture for the parsed advertisement (and @pytest.mark.parametrize for the empty-mfr-data case) would shrink the file and make future additions cleaner.

Not a blocker — the duplication is bounded and the intent is clear.

2. Consider also asserting attempt_state is None in the empty-mfr-data case (`tests/test_keypad.py`, L42-58)

test_keypad_advertisement_battery_none_when_no_data currently only checks that get_battery_percent() is None. Since process_wokeypad returns {"battery": None, "attempt_state": None} together when mfr_data is None (see adv_parsers/keypad.py:15-16), it would be cheap to also assert device.parsed_data["attempt_state"] is None to fully exercise that branch and document the contract.

assert device.get_battery_percent() is None
assert device.parsed_data["attempt_state"] is None
3. Optional: reference SwitchbotModel.KEYPAD in the docstring (`switchbot/devices/keypad.py`, L7-14)

The class body is intentionally empty (correct for a passive BLE device). To help discoverability — especially relative to the very similarly named SwitchbotKeypadVision (which is an encrypted, command-capable device) — consider mentioning SwitchbotModel.KEYPAD and contrasting with the Vision variant in the class docstring. Purely cosmetic.


Checklist

  • Class follows pattern of other passive device classes
  • Public export added in alphabetical order
  • Tests exercise the full parse_advertisement_data pipeline
  • None-handling branch covered — suggestion #2
  • No hardcoded secrets, unsafe deserialization, or injection vectors
  • Coverage of new code
  • Python 3.11+ compatible

Summary

Small, well-scoped change that fills a real gap: the process_wokeypad parser and SwitchbotModel.KEYPAD enum already existed, but no device class consumed them. Implementation is correctly minimal (passive BLE — no commands), the public export is alphabetized, and codecov reports 100% coverage on the new code. The author addressed the prior automated review by driving tests through parse_advertisement_data with real BLE bytes and asserting on get_battery_percent() rather than stuffing pre-parsed values — that's the right call and the tests now actually exercise the parser path end-to-end. Author flagged that the code wasn't tested against real hardware, but the byte-layout is the same as the existing test_parse_advertisement_data_keypad fixture in test_adv_parser.py:1450, so regressions would surface. Merge-ready; the inline comments are nits.


Automated review by Kōan570b216
7430fd3
72f509b
09dd94e
e2174d0
0d4085c
d0167e8

Tests previously injected pre-parsed values, bypassing the parser.
Now exercises parse_advertisement_data end-to-end so byte-layout
regressions in adv_parsers/keypad.py are caught.

- Assert get_battery_percent() instead of raw dict access
- None test exercises parser's own None branch (empty mfr_data)
- Extract make_advertisement_data to tests/__init__.py; remove
  duplicate in test_keypad_vision.py
- Expand SwitchbotKeypad docstring: passive-only, no commands
@italo-lombardi
Copy link
Copy Markdown
Author

Addressed all suggestions from the automated review:

  1. Tests now exercise the full pipeline — rewrote all three tests to call parse_advertisement_data with real BLE bytes instead of injecting pre-parsed values. Added assertion on get_battery_percent().

  2. Fixed misleading testtest_keypad_advertisement_battery_none_when_no_data now passes empty manufacturer_data so the None comes from the parser's own None-handling branch (adv_parsers/keypad.py:15-16), not a manual override.

  3. Removed copy-paste — moved make_advertisement_data to tests/__init__.py as a shared helper. test_keypad_vision.py now imports it from there.

  4. Added docstring to SwitchbotKeypad — clarifies passive-only BLE, no commands, and points to adv_parsers/keypad.py for data origin.

  5. Fixture bytesKEYPAD_INFO in tests/__init__.py is already the single source of truth; the new tests reference it directly, so test_adv_parser.py could be updated to do the same in a follow-up if desired.

@italo-lombardi
Copy link
Copy Markdown
Author

@bluetoothbot review

@bluetoothbot
Copy link
Copy Markdown
Collaborator

❌ Permission denied. Only users with write access can trigger bot commands.

File was not in scope for this PR. Revert changes to it and
remove the shared make_advertisement_data helper from tests/__init__.py
that was added only to support that refactor.
@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 14, 2026

@bluetoothbot review

italo-lombardi and others added 2 commits May 15, 2026 10:10
Completes coverage of the None-handling branch in process_wokeypad:
both battery and attempt_state are None when manufacturer data absent.
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.

4 participants