SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
269 stars 462 forks source link

Update the doConfigure Logic in Matter Thermostat #1662

Closed hcarter-775 closed 1 month ago

hcarter-775 commented 1 month ago

Check all that apply

Type of Change

Checklist

Description of Change

This should be a pure refactor of the current doConfigure logic in the Matter Thermostat driver. Right now, lots of similar code is run in multiple places, and is confusing. Small changes have a greater capacity for error due to the multiple branches currently in use. This change aims to shrink all of this, without changing any final profile configurations at all.

Summary of Completed Tests

Unit tests continue to pass. VDA devices continue to onboard as expected.

github-actions[bot] commented 1 month ago

Channel deleted.

github-actions[bot] commented 1 month ago

Test Results

   63 files    395 suites   0s :stopwatch: 1 934 tests 1 934 :white_check_mark: 0 :zzz: 0 :x: 3 358 runs  3 358 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 9e010f99.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 1 month ago

File Coverage
All files 82% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/embedded-cluster-utils.lua 42% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 85% :x:

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against 9e010f99f42b8f8dcf88314995413544a4fc1615

nickolas-deboom commented 1 month ago

I like the idea of separating the profile creation logic into functions for each device type. That really helps make it easier to follow. These changes look good to me, nice work!

nickolas-deboom commented 1 month ago

I was just going to add that some test cases might be useful to test the new code paths, but I looked through the existing test cases in the driver and there seems to already be a lot of test cases for profile selection, which are still passing after these changes so I'd say that it's not necessary.