Skip to content

Conversation

@helto4real
Copy link
Collaborator

@helto4real helto4real commented Apr 29, 2023

Breaking change

This PR introduces some breaking changes. The rationale for these changes is to allow more granular subscriptions to events and remove default subscriptions to all events for users of IHomeAssistantConnection.

Most users does not use the HassClient and should not be affected by this change.

IHomeAssistantConnection

  • ProcessHomeAssistantEventsAsync method no longer defaults to subscribing to all event changes. That logic is moved the new method SubscribeToHomeAssistantEventsAsync that returns the IObservable<HassEvent> instead of make it available as a property on the interface itself.
  • OnHomeAssistantEvent is removed and replaced by the method SubscribeToHomeAssistantEventsAsync

ServiceCollectionExtensions

  • IObservable<HassEvent> is no longer injectable through DI due to internal design change providing it through the SubscribeToHomeAssistantEventsAsync on the IHomeAsstantConnection interface instead.

Proposed change

This PR refactors how event subscriptions work. Now it does not automatically subscribe to all events from the HomeAssistantConnection. The runtime and HassModel do add these subscriptions automatically. The function now also supports subscribing to specific avents.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • The code compiles without warnings (code quality chek)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@helto4real
Copy link
Collaborator Author

helto4real commented Apr 29, 2023

Still have a few tests I want to add but we can discuss the overall design first. I also have the nested task thing to take care of.
[edit, all tests and refactorings are now included and the PR is ready for review]

@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Patch coverage: 93.47% and project coverage change: +0.04 🎉

Comparison is base (72a0ed4) 80.70% compared to head (9c32ee9) 80.74%.

❗ Current head 9c32ee9 differs from pull request most recent head 5dc6874. Consider uploading reports for the commit 5dc6874 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #868      +/-   ##
==========================================
+ Coverage   80.70%   80.74%   +0.04%     
==========================================
  Files         167      168       +1     
  Lines        4229     4244      +15     
  Branches      423      423              
==========================================
+ Hits         3413     3427      +14     
  Misses        642      642              
- Partials      174      175       +1     
Flag Coverage Δ
unittests 80.74% <93.47%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nt/Common/Extensions/ServiceCollectionExtension.cs 97.56% <ø> (-0.06%) ⬇️
...mon.HassClient/Internal/HomeAssistantConnection.cs 87.21% <91.89%> (+1.15%) ⬆️
...NetDaemon.HassClient/Internal/Helpers/AsyncLazy.cs 100.00% <100.00%> (ø)
...tDaemon.HassClient/Internal/HomeAssistantRunner.cs 88.42% <100.00%> (ø)
...el/NetDeamon.HassModel/Internal/EntityAreaCache.cs 75.00% <100.00%> (-2.28%) ⬇️
...l/NetDeamon.HassModel/Internal/EntityStateCache.cs 87.17% <100.00%> (ø)
...time/NetDaemon.Runtime/Internal/AppStateManager.cs 87.14% <100.00%> (+0.37%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@helto4real helto4real marked this pull request as ready for review April 29, 2023 18:40
@helto4real helto4real force-pushed the refactor-subscription branch 4 times, most recently from 68a6139 to beb718e Compare May 1, 2023 07:56
@helto4real helto4real force-pushed the refactor-subscription branch 2 times, most recently from 9c32ee9 to f0cd95e Compare May 4, 2023 15:33
@helto4real
Copy link
Collaborator Author

I missed a few spots in the tests and also fixed old tests that had the same problem. I will merge this now. Thanks for the review @FrankBakkerNl

@helto4real helto4real force-pushed the refactor-subscription branch from f0cd95e to 5dc6874 Compare May 4, 2023 16:14
@helto4real helto4real merged commit b9a80aa into dev May 4, 2023
@helto4real helto4real deleted the refactor-subscription branch May 4, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants