-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Add services for jewish_calendar integration #111986
base: dev
Are you sure you want to change the base?
Conversation
Hey there @tsvi, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
I'm going to add parameters to specify a partial Hebrew date |
26286f4
to
514dcab
Compare
I completely rewrote this; it's now ready. I'll write docs soon (WDYT of the exposed API?) |
Updated docs in home-assistant/home-assistant.io#31770 |
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 great. Please rename the services and I'll approve.
Thanks a lot
"for_gregorian_date": "mdi:calendar", | ||
"for_hebrew_date": "mdi:calendar", | ||
"get_holidays": "mdi:calendar-star", | ||
"for_next_holiday": "mdi:calendar-arrow-right" |
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.
As mentioned in the documentation, rename these to get_*
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.
Done
@@ -252,10 +253,19 @@ def get_state( | |||
# Compute the weekly portion based on the upcoming shabbat. | |||
return after_tzais_date.upcoming_shabbat.parasha | |||
if self.entity_description.key == "holiday": | |||
self._attr_device_class = SensorDeviceClass.ENUM |
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.
Great catch 👍
This should also be blocked on py-libhdate/py-libhdate#117, which unbreaks |
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.
LGTM. Will try and release the fix for py-libhdate sometime next week so we don't have the double Purim bug you mentioned.
Released py-libhdate v0.10.7. Please update to this version to deal with the issue of double purim. |
Done |
I can't figure out why this breaks the typing in sun/init.py. The type of the property being set did not change. |
That's my bad, I updated the astral dependency in the library in preparation for potentially adding calculations about the moon. |
Please retry with version v0.10.8 |
Done |
Unfortunately, you'll have to fix the tests. Although some changes were accepted into the master branch, a new version wasn't released since 2021. So, there are some minor fixes to be dealt with. Sorry. |
Yes; will do soon |
Fixed |
@MartinHjelmare This is ready for review by the core team or whomever has commit rights. |
Can someone please approve this? |
Can someone please approve this? |
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.
Please split this PR in three PRs:
- Bump the library version
- Add services to return information about arbitrary Hebrew dates and holidays.
- Make Jewish Calendar Holiday an enum sensor, to show a dropdown list when using it in conditions.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
This is now ready |
selector: | ||
select: | ||
options: | ||
- Tishrei |
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.
Is there a way we can provide this list from py-libhdate? What happens if we start supporting more languages? Other spellings?
The same is especially true for holidays.
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.
It looks like this is possible via async_set_service_schema()
, which is meant to register dynamically-generated services.
However, looking through its implementation, it ought to work fine to replace an already-registered service too, which means I should be able to call it to either supplement or entirely replace services.yaml
here.
Do you think core is likely to accept that?
@emontnemery Done |
Breaking change
Proposed change
Adds services to return information about arbitrary Hebrew dates and holidays.
This makes it possible to build automations that react to future calendar events.
Also make
Jewish Calendar Holiday
an enum sensor, to show a dropdown list when using it in conditions.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: