8000 Google calendar by mnestor · Pull Request #2052 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Google calendar #2052

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

Closed
wants to merge 10 commits into from
Closed

Google calendar #2052

wants to merge 10 commits into from

Conversation

mnestor
Copy link
Contributor
@mnestor mnestor commented May 12, 2016

Description:
Adds Google Calendar Event Devices and a generic Calendar Device for future calendar integration.
@Bart274 is working on iCloud calendars and will be utilizing the generic Calendar Device.

Fixes comments from previous PR #1982
Require other PR for Configurator changes #2035
Documentation: home-assistant/home-assistant.io#447

Apologies about the new PR, I completely messed up my fork and had to restart it.

Related issue (if applicable): #

Example entry for configuration.yaml (if applicable):

google_calendar:
  track_new_calendar: yes  # optional, defaults to yes

Checklist:

If code communicates with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
    Fails
E:299,20: Instance of 'Resource' has no 'calendarList' member (no-member)
E:358,13: Instance of 'Resource' has no 'events' member (no-member)
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@@ -0,0 +1,389 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this component google. Yes, for now it's only requesting calendar oauth scope but we could extend this to other scopes with support in other components in the future.

@balloob
Copy link
Member
balloob commented May 13, 2016

So this PR is quite blowing up…

I would suggest to not include the automation trigger. Small steps at a time. Create a new PR once this once is merged. If you want to keep it in, you need to write tests for it to validate that it works. It also shouldn't just trigger every minute but instead listen to state changes of the entity id, that will be more efficient.

Also new components need to be accompanied with a demo platform and test coverage for all offered functionality.

@mnestor
Copy link
Contributor Author
mnestor commented May 13, 2016

Not sure how I'd create a demo/test because of the oauth that needs to happen.

As far as the trigger happening every minute I was trying to avoid state changes every minute. Since someone could potentially change the event after it's registered here the offset could change causing the trigger to fire at the wrong time.

Event: Test #Notify #-10, Start: In an hour
Trigger registered to fire in 50 minutes

Change event to Start in 2 hours
Trigger still registered to fire in 50 minutes, should be in 1:50 minutes.

If you can point me in the direction of how to implement a dynamic target for the trigger without checking every minute I'd appreciate it! I was doing a minute countdown in the state but that's noisy and logs a lot to the db as you pointed out. This is where I went next. The offset trigger can be changed at any time which makes this difficult for me to grasp.

@mnestor
Copy link
Contributor Author
mnestor commented May 13, 2016

Just thought of how to do it. Watch for state changes to start_time and offset_time and set a specific time listener and hold onto the ref to that so I can remove/reset it if those change.

I'll pull it for now. Have to spend the weekend learning tests. Still clueless on the demo part though.

@mnestor mnestor force-pushed the google_calendar branch from a2d2550 to 9675098 Compare May 13, 2016 12:26
@balloob
Copy link
Member
balloob commented May 13, 2016

The tests would be for the calendar component. You can skip testing all of
Google.

But since the calendar component needs a platform to provide get next event
for tests, that's what the demo component would implement. If you look at
any other demo platform (light, switch etc) you'll see what I mean.

On Fri, May 13, 2016, 05:14 mnestor notifications@github.com wrote:

Just thought of how to do it. Watch for state changes to start_time and
offset_time and set a specific time listener and hold onto the ref to that
so I can remove/reset it if those change.

I'll pull it for now. Have to spend the weekend learning tests. Still
clueless on the demo part though.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#2052 (comment)

@mnestor
Copy link
Contributor Author
mnestor commented May 15, 2016

There's a bug I'm working on, can't keep the start/end as a datetime in the state.

@balloob
Copy link
Member
balloob commented May 15, 2016

You can call isoformat() on any datetime to serialize it, and use util.dt.parse_datetime to get it

@mnestor
Copy link
Contributor Author
mnestor commented May 15, 2016

Yeah, I've done that everywhere now and still getting the error.

@mnestor
Copy link
Contributor Author
mnestor commented May 15, 2016

I was overthinking it. sigh Not sure why recorder can't do this itself. Seems like it'd be useful to have the datetime in the state for automation work comparing to now()

@property
def state_attributes(self):
8000 """State Attributes for HA."""
start = self._cal_data.get('start', None)
Copy link
Member

Choose a reason for hiding this comment

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

You're not using these variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brain fart, supposed to return those, fixed. Had to copy from my prod system since I couldn't replicate in my dev.

@mnestor mnestor force-pushed the google_calendar branch from f8c905b to 18e41d3 Compare May 15, 2016 20:58
@mnestor mnestor closed this May 21, 2016
@mnestor mnestor mentioned this pull request Oct 31, 2016
8 tasks
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0