Matter Switch: Refactor Driver File Organization #2401
Conversation
hcarter-775
requested review from
HunsupJung,
cjswedes,
ctowns,
greens,
nickolas-deboom and
tpmanley
|
Channel deleted. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against eee6f89 |
Test Results 71 files 452 suites 0s ⏱️ Results for commit eee6f89. ♻️ This comment has been updated with latest results. |
greens
left a comment
greens
left a comment
There was a problem hiding this comment.
quick question: Are there any hubcore versions out there in production that don't have these clusters in their lua_libs still?
There was a problem hiding this comment.
missing copyright statement
Yes, there is a non-zero percentage. We are still watching these numbers to see when is appropriate to remove these altogether, but for now have elected to keep them in the driver. |
| clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported | ||
| } | ||
| } | ||
| local attribute_handlers = require "generic-handlers.attribute_handlers" |
There was a problem hiding this comment.
nit: IMO we should have our sub folders using snake_case, but its sort of the wild west; the lua libs follow this, but we dont for the driver and subdriver directory names ¯\(ツ)/¯
There was a problem hiding this comment.
100% agree, just didn't want to rock the boat too hard
hcarter-775
force-pushed
the
refactor-switch-driver
branch
from
52bfc0d to
eaba018
Compare
ctowns
left a comment
ctowns
left a comment
There was a problem hiding this comment.
This looks good to me - could you provide a little more commentary on the testing you did in the Summary of Completed Testing section. Did your spot testing include testing all capabilities, driver restart, etc.? Also, do we have coverage of all device types (have we tested an energy device)? If not, I would call out if any device types are covered by unit tests only. But, the refactor makes sense to me and I assume the unit tests would fail if there was any obvious problem, so overall I think this looks good to me.
Great job with the refactor, this looks very well organized!
drivers/SmartThings/matter-switch/src/generic_handlers/capability_handlers.lua
Show resolved
Hide resolved
nickolas-deboom
left a comment
nickolas-deboom
left a comment
There was a problem hiding this comment.
Looks good to me besides the comment I left about the embedded clusters!
Description of Change
This PR aims to re-organize the Matter Switch driver to be easier to read and more self-documenting in its nature.
It does not change any current handling at all, and only re-organizes where the variables and functions are defined, that is, moving them under different files and meta-tables.
The general directory structure is as follows, within
src/:embedded-clustersgeneric-handlerssub-driversutilsSummary of Completed Tests
Ensured capability and attribute handlers ran on the following VDA device types:
This testing does not handle an exhaustive list of all handlers that could be hit since this driver includes MCD devices that have illuminance, occupancy, motion, fans, etc. To try every combination is out of scope for this refactor. However, this spot testing, plus a series of unit tests hitting every handler ensures that no mapping between handler and attribute was missed, nor that anything was unduly erased during the refactor. Further, I have restarted the driver multiple times with some of these device types (OnOff Switch, Dimmer Switch) and have seen functionality remain constant.
Since this just moves code around, the primary worry here is that something was deleted or that some typo has come up, which breaks some piece of the code. However, since all tested devices do continue to work and all unit tests continue to pass, I am confident that this has not happened.
Further, I have scanned each handler manually and using inline VScode toolsand ensured each map is expected. I have also manually stepped through all introduced
fields.blankandswitch_utils.blankcalls and ensured they all map to something meaningful.Last, I have been able to confirm through unit tests with a backend of older lua libs that the embedded cluster definitions continue to run as expected.