Skip to content

Conversation

@spelech
Copy link
Contributor

@spelech spelech commented Jan 16, 2023

Proposed change

Modify the CronExtensions to handle cron strings with seconds specified. Cronos supports this but the underlying call to Parse needs to specify the format beforehand. I opted to add a default boolean to the existing call to avoid a breaking change.

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

None

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass -Adding addl test
  • 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:

@spelech spelech marked this pull request as ready for review January 16, 2023 03:50
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2023

Codecov Report

Base: 81.10% // Head: 80.84% // Decreases project coverage by -0.27% ⚠️

Coverage data is based on head (40cac1f) compared to base (94d6465).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 40cac1f differs from pull request most recent head 7a80276. Consider uploading reports for the commit 7a80276 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #828      +/-   ##
==========================================
- Coverage   81.10%   80.84%   -0.27%     
==========================================
  Files         159      159              
  Lines        4106     4092      -14     
  Branches      405      404       -1     
==========================================
- Hits         3330     3308      -22     
- Misses        616      621       +5     
- Partials      160      163       +3     
Flag Coverage Δ
unittests 80.84% <100.00%> (-0.27%) ⬇️

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

Impacted Files Coverage Δ
.../NetDaemon.Extensions.Scheduling/CronExtensions.cs 95.45% <100.00%> (+0.21%) ⬆️
...mon.HassClient/Internal/Net/WebSocketClientImpl.cs 61.11% <0.00%> (-8.34%) ⬇️
...Model.CodeGenerator/Extensions/StringExtensions.cs 93.75% <0.00%> (-6.25%) ⬇️
...l.CodeGenerator/CodeGeneration/ServiceArguments.cs 90.62% <0.00%> (-3.97%) ⬇️
...MetaData/EntityMetaData/EntityMetaDataGenerator.cs 95.00% <0.00%> (-2.15%) ⬇️
...mon.HassClient/Internal/HomeAssistantConnection.cs 87.06% <0.00%> (-1.73%) ⬇️
....CodeGenerator/CodeGeneration/EntitiesGenerator.cs 100.00% <0.00%> (ø)
...nsions/HomeAssistantConnectionHelpersExtensions.cs
...istant/Commands/CreateInputBooleanHelperCommand.cs
...t/Common/HomeAssistant/Model/InputBooleanHelper.cs
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

count.Should().Be(3, because: "Action should not fire after Dispose()");
#endregion

#region Cron with Seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a separate test instead of regions?

Also this does not really test the seconds really work, but you could argue that is a feature or Cron.Net and not this extansion

@FrankBakkerNl FrankBakkerNl enabled auto-merge (squash) February 6, 2023 14:49
@FrankBakkerNl FrankBakkerNl merged commit 4be27c5 into net-daemon:dev Feb 6, 2023
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.

5 participants