-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
Google Calendar round 2 #4161
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
Google Calendar round 2 #4161
Conversation
from homeassistant.helpers.entity import Entity | ||
from homeassistant.const import (STATE_ON, STATE_OFF) | ||
from homeassistant.util import Throttle | ||
from homeassistant.components import ( |
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.
- 1: F401 'homeassistant.components.google' imported but unused
""" | ||
import os | ||
import logging | ||
import socket |
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.
- 1: F401 'socket' imported but unused
import os | ||
import logging | ||
import socket | ||
from datetime import timedelta |
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.
- 1: F401 'datetime.timedelta' imported but unused
import homeassistant.helpers.config_validation as cv | ||
import homeassistant.loader as loader | ||
from homeassistant import bootstrap | ||
from homeassistant.const import HTTP_OK |
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.
- 1: F401 'homeassistant.const.HTTP_OK' imported but unused
hass, 'In order to authorize Home-Assistant to view your calendars' | ||
'You must visit: <a href="{}" target="_blank">{}</a> and enter' | ||
'code: {}'.format(devFlow.verification_url, | ||
devFlow.verification_url, devFlow.user_code), |
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.
- 17: E128 continuation line under-indented for visual indent
title=NOTIFICATION_TITLE, notification_id=NOTIFICATION_ID) | ||
|
||
listener = track_time_change(hass, step2_exchange, | ||
second=range(0, 60, devFlow.interval)) |
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.
- 9: E128 continuation line under-indented for visual indent
for calendar_hash, calendar in calendars.items(): | ||
_CALENDARS.update({calendar_hash: calendar}) | ||
discovery.load_platform(hass, 'calendar', DOMAIN, | ||
_CALENDARS[calendar_hash]) |
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.
- 9: E128 continuation line under-indented for visual indent
_CALENDARS[calendar_hash] | ||
) | ||
|
||
discovery.load_platform(hass, 'calendar', DOMAIN, _CALENDARS[calendar_hash]) |
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.
- 80: E501 line too long (80 > 79 characters)
0a40c39
to
7c20cd5
Compare
Could we have the ability to specify a list of calendars we want to track? Also, thanks for continuing this! |
@lwis It's there, it creates a google_calendars.yaml file similar to known_devices.yaml and also allows you to specify search based. But I just realized I messed that up when I tried to make it so you could specify the device name. Crap.
|
@mnestor one comment, if search is null we shouldn't write/display it, just make sure that it's documented on the website. |
7c20cd5
to
5a466a4
Compare
Changed the google_calendars.yaml around
The above would only create one calendar, a search based one in you holiday calendar to turn on only on Christmas Day. |
7225626
to
2c4038d
Compare
@mnestor I'll take a look at how I can use your component to add an icloud calendar as well :-) |
@Bart274 Nothing has changed since we last talked. I've just found a new way to do the oauth for the google part that makes it a LOT easier for people to get setup. |
@mnestor I see, I will look how I can add the icloud calendar :)
|
ec6c57f
to
4eaf1b8
Compare
REQUIREMENTS = [ | ||
'google-api-python-client==1.5.4', | ||
] | ||
DEPENDENCIES = ['calendar'] |
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.
The Google component shouldn't depend on calendar.
@property | ||
def track(self): | ||
"""Are we tracking events on this search sensor.""" | ||
return self._track |
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.
What does this mean? If it shouldn't track anything it shouldn't have been loaded to begin with.
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.
Yeah, that's dumb. Copy/paste I never thought about.
self._end_listener = track_point_in_time(self.hass, _end, | ||
self._cal_data['end']) | ||
|
||
def value_changed(self, state): |
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.
Doesn't seem to be used?
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 appeared, months ago, that this was called by another process and I didn't have to deal with changing the state. I take it either I'm wrong about that and this was never needed?
Do I need to call update_ha_state when state data changes?
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.
Home Assistant will automatically call update every 60 seconds (as that's what you entered into the EntityComponent constructor).
"""Return the device name.""" | ||
return self._name | ||
|
||
@property |
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.
This is not being used outside of this entity. You can just have other pieces of code directly refer to the internal attribute self._state
. I also think that self._state
could be renamed to better describe what the value means that it holds (I'm assuming it represents if an event is currently happening?).
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.
I'm confused. I thought this was needed so I could check it in an automation?
- condition: state
entity_id: calendar.name
state: 'on'
I left it as on/off since it made sense to me as these are basically binary switches for being in an event state.
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.
None of the properties that are defined on an entity will be set as state attributes. That's what state_attributes
is for.
def _found_calendar(call): | ||
"""Check if we know about a calendar and generate PLATFORM_DISCOVER.""" | ||
calendar = get_calendar_info(hass, call.data) | ||
if _CALENDARS.get(calendar[CONF_CAL_ID], None) is not None: |
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 have added hass.data
as a dictionary for components to store data. You will no longer need to use globals. Make sure you make the key unique to your component, ie hass.data['google_calendars']
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.
Thank you
@@ -17,6 +17,7 @@ | |||
COMPONENTS_WITH_DEMO_PLATFORM = [ | |||
'alarm_control_panel', | |||
'binary_sensor', | |||
'calendar', |
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.
A demo platform has not been included?
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 got lost when I built a new system to work on this on. I'll get it added back.
DEPENDENCIES = ['calendar'] | ||
|
||
_LOGGER = logging.getLogger(__name__) | ||
_CONFIGURING = {} |
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.
This doesn't seem to be used.
""" | ||
raise NotImplementedError(message) | ||
|
||
@Throttle(MIN_TIME_BETWEEN_UPDATES) |
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 never throttle at the entity level. It is up to the platforms to decide how much they require throttling. For example the demo platform will never need a throttle.
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.
I think I've changed it to how you want it.
a8dc664
to
22ddc68
Compare
|
||
TOKEN_FILE = '.{}.token'.format(DOMAIN) | ||
|
||
PLATFORM_SCHEMA = vol.Schema({ |
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.
This is a component, so you should use CONFIG_SCHEMA
. See group example
def setup(hass, config): | ||
"""Setup the platform.""" | ||
config = config.get(DOMAIN, {}) | ||
if isinstance(config, list) and len(config) > 0: |
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.
This will no longer ben ecessary when you define CONFIG_SCHEMA
instead of PLATFORM_SCHEMA
bootstrap.setup_component(hass, 'calendar', config) | ||
|
||
for calendar in hass.data[DATA_INDEX].values(): | ||
discovery.load_platform(hass, 'calendar', DOMAIN, calendar) |
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 calendar JSON serializable?
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.
Yes
|
||
def update_config(path, calendar): | ||
"""Write the google_calendar_devices.yaml.""" | ||
with threading.Lock(): |
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.
This will create a new lock every time you call this, so that means that the lock is never shared -> the locking won't work
@@ -28,6 +28,9 @@ omit = | |||
homeassistant/components/envisalink.py | |||
homeassistant/components/*/envisalink.py | |||
|
|||
homeassistant/components/google.py |
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.
No need to exclude from coverage if you wrote tests ?
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.
@balloob how would I go about disabling coverage for the google api stuff? Since I'm not failing for those.
22ddc68
to
1f3bc08
Compare
1f3bc08
to
044642a
Compare
Description:
Creating a new pull since #2052 was closed so long ago.
Adds Google Calendar Event Devices and a generic Calendar Device for future calendar integration.
I've set this up after discussions with @Bart274 about doing an iCloud calendar similar to this so it should be easier to add in that support after this.
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#1403
Example entry for
configuration.yaml
:Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable ([example][ex-requir]).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass