Guide users to use WhenStateIsFor with a IScheduler instance.

[Rick-van-Dam]

Breaking change

Proposed change

This is a Source compatible change that will guide users to provide a ISchedule instance to WhenStateIsFor. This is however a binary breaking change but I think that might be fine as users will recompile anyway after updating netdaemon.

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 Guide users to use WhenStateIsFor with IScheduler parameter #801
• 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:

• Documentation added/updated for www.home-assistant.io

[codecov]

Codecov Report

Patch coverage: 55.55% and project coverage change: -0.09 ⚠️

Comparison is base (e65e34d) 80.79% compared to head (dd05f4f) 80.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #864      +/-   ##
==========================================
- Coverage   80.79%   80.70%   -0.09%     
==========================================
  Files         167      167              
  Lines        4217     4229      +12     
  Branches      419      423       +4     
==========================================
+ Hits         3407     3413       +6     
- Misses        640      642       +2     
- Partials      170      174       +4     

Flag	Coverage Δ
unittests	80.70% <55.55%> (-0.09%)	⬇️

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

Impacted Files	Coverage Δ
...l/NetDeamon.HassModel/StateObservableExtensions.cs	68.00% <55.55%> (-16.62%)	⬇️

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

[helto4real]

Not sure why you want to obsolete this? Please explain.

[Rick-van-Dam]

Passing in a injected IScheduler is the more correct approach which is also unit testable. I think it confuses ppl currently since there were also questions about this on Discord.

However since ppl might be using WhenStateIsFor without IScheduler currently it will be breaking if you completely remove it so I didnt and just made the one without a IScheduler parameter obsolete.

[FrankBakkerNl]

We discussed this some time ago when Rick created the issue for this. I agree it is preferred for several reasons to pass the Scheduler. This will now give a warning when you don't.

Someone might have a case where not passing the scheduler is 'ok'. Maybe if you only ever have short times where it is no problem if the event fires after the app unloads AND are not interested in Unit testing.

So the trade off is between preventing user from a mistake vs forcing users to pass a scheduler while it's not really needed.

[helto4real]

We discussed this some time ago when Rick created the issue for this. I agree it is preferred for several reasons to pass the Scheduler. This will now give a warning when you don't.

Someone might have a case where not passing the scheduler is 'ok'. Maybe if you only ever have short times where it is no problem if the event fires after the app unloads AND are not interested in Unit testing.

So the trade off is between preventing user from a mistake vs forcing users to pass a scheduler while it's not really needed.

I will not have user "have to" pass it. Warning should be ok. Not worth it to make it a breaking change. What you think @FrankBakkerNl ? Should we go with this?

[FrankBakkerNl]

I think so, if ever we would change our minds we can just remove the obsolete attribute