-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jewish calendar binary sensor #26200
Conversation
8d5352b
to
085866e
Compare
Updates for PR home-assistant/core#26200
6d0af80
to
b105c7d
Compare
Finally got all the tests to pass. @MartinHjelmare any more fixes necessary? |
As part of this, move tests to use async_setup_component instead of testing JewishCalendarSensor as suggested by @MartinHjelmare here: home-assistant#24958 (review)
…_time_zone in tests
This reverts commit 74371740eaeb6e73c1a374725b05207071648ee1.
6886e4e
to
879e2f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixed the imports as requested |
I think we need a breaking change label here. |
Please add to the breaking change paragraph what the user needs to do to cope with the breaking change. |
@MartinHjelmare can you merge the PR? Or are we waiting for another review? |
We're all good! Changing the PR description doesn't generate a github notification. |
Breaking Change:
Change in
configuration.yaml
To
Automations based on
sensor.issur_melacha_in_effect
comparison toTrue
/False
need to be updated to usebinary_sensor.issur_melacha_in_effect
on and off states.Explanation
The
issur_melacha_in_effect
was supposed to be a binary sensor. I wasn't aware of the distinction back when I first developed this integration. This fixes this issue by creating a new platform, supporting both types of sensors.Description:
Make Jewish calendar its own platform with two types of sensors: sensor and binary sensor. In a future PR I plan to split the sensor furthermore in data and time related so that I can provide datetime attributes in the same fashion as the date sensor. This will provide easier ways of writing automations.
Related issue (if applicable): fixes #20315
As part of this change, I implemented @MartinHjelmare suggestion for testing the components,
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10231
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
.If the code does not interact with devices: