-
-
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
Jewish calendar sensor #16393
Jewish calendar sensor #16393
Conversation
|
||
from homeassistant.components.sensor import PLATFORM_SCHEMA | ||
|
||
import homeassistant.helpers.config_validation as cv |
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.
'homeassistant.helpers.config_validation as cv' imported but unused
https://home-assistant.io/components/sensor.jewish_calendar/ | ||
""" | ||
import logging | ||
from datetime import date as dt |
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.
'datetime.date as dt' imported but unused
Will need to update hdate API and version before continuing.
Could not find a matching PR in the documentation. Added the |
CONF_SENSORS = 'sensors' | ||
|
||
DEFAULT_NAME = 'Jewish Calendar' | ||
DEFAULT_LATITUDE = 31.778 |
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.
We should default to the users location
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.
Oh, you already do that. In that case, please remove these as they serve no purpose.
|
||
def __init__(self, name, language, sensor_type, | ||
latitude=DEFAULT_LATITUDE, longitude=DEFAULT_LONGITUDE, | ||
diaspora=False): |
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.
Should be no defaults here, defaults should be set in voluptuous schema.
"""Update the state of the sensor.""" | ||
import hdate | ||
|
||
self._date = dt_util.now().date() |
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.
There is no reason to store this as an instance variable, it's only used in this method.
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.
Thanks 🐦
THANK YOU!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! |
Why is it disapora and not diaspora? |
Description:
This adds a Jewish calendar sensor to home assistant.
By default it displays the current Hebrew calendar date in English characters.
G-d willing, I'll be adding more sensors in the future and cleanup the API for hdate so the interface between Hass and hdate can be done in a cleaner fashion.
At the moment I implemented the most interesting sensors.
Also on the TODO list is changing the Hebrew date at sunset as is customary in Jewish practice.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6265
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:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: