Skip to content
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

Rewrite calendar component #24950

Merged
merged 6 commits into from Jul 11, 2019

Conversation

@MartinHjelmare
Copy link
Member

commented Jul 4, 2019

Breaking Change:

The entity state attributes are not reset when the calendar event has passed. The last event attributes will remain until new calendar event info is generated by the integration. Automations that rely on state attributes getting reset will need to be updated.

Description:

  • The calendar integration has been rewritten to follow our current standards. This is the first step in solving home-assistant/architecture#38.
  • The change is mostly non breaking. The reset (clean up) of state attributes upon an event time passing has been removed though. It seemed weird to reset the attributes to some arbitrary default values instead of keeping the attributes representing the last event.
  • All platforms have been converted.

Changes:

  • Save entity component in hass.data.
  • Rename device_state_attributes to state_attributes in base calendar entity.
  • Remove offset attribute from base state_attributes.
  • Extract offset helpers to calendar component.
  • Clean imports.
  • Remove stale constants.
  • Remove name and add async_get_events.
  • Add normalize_event helper function. Copied from #21495.
  • Add event property to base entity.
  • Use event property for calendar state.
  • Ensure event start and end.
  • Remove base entity init.
  • Temporary keep old start and end datetime format.
  • Convert demo calendar.
  • Convert google calendar.
  • Convert caldav calendar.
  • Convert todoist calendar.

Related issue (if applicable):
home-assistant/architecture#38

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

MartinHjelmare added some commits Apr 28, 2019

Rewrite calendar component
* Save component in hass.data.
* Rename device_state_attributes to state_attributes.
* Remove offset attribute from base state_attributes.
* Extract offset helpers to calendar component.
* Clean imports.
* Remove stale constants.
* Remove name and add async_get_events.
* Add normalize_event helper function. Copied from #21495.
* Add event property to base entity.
* Use event property for calendar state.
* Ensure event start and end.
* Remove entity init.
* Add comment about event data class.
* Temporary keep old start and end datetime format.
Convert google calendar
* Convert google calendar.
* Clean up google component.
* Keep offset feature by using offset helpers.
Convert caldav calendar
* Clean up caldav calendar.
* Update caldav cal on addition.
* Bring back offset to caldav calendar.
* Copy caldav event on update.
@ghost

This comment has been minimized.

Copy link

commented Jul 4, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (demo) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

I'm marking this as draft since I'd like feedback and there's an architecture issue linked. The change is basically ready though. We might want to add tests for the demo platform as there currently are none.

I haven't removed the entity representation as I don't think it's clear that that would be the best way forward. I also wanted to keep the change mostly non breaking.

'description': self._cal_data.get('description', None),
'message': event['message'],
'all_day': event['all_day'],
'start_time': event['start'],

This comment has been minimized.

Copy link
@balloob

balloob Jul 10, 2019

Member

I am not happy with the normalization, however we should probably not make that change in this PR.

There are two types of events (per RFC5545): all day and non-all day. All day events only have a start and end date, so that they are not impacted by timezones. Non-all day have start and end datetimes, so that changing timezone can adjust the event. It looks like we convert all day events to start/end times.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 11, 2019

Author Member

Yes, all start and end are converted to datetime. We have the boolean 'all_day' set to signal all day event.

I was thinking about making a data class, eg with attrs, with specified attributes, where an instance of this class should be returned as the event data, instead of having platforms create a dict with the event data. Then we can better control what the event data should be.

But I decided not to do that in this PR, but save it for later.

This comment has been minimized.

Copy link
@balloob

balloob Jul 11, 2019

Member

That is a great idea. And yeah, future PR 👍

self._cal_data['start'] = start
self._cal_data['end'] = end
self._cal_data['all_day'] = 'date' in self.data.event['start']
async def async_get_events(self, hass, start_date, end_date):

This comment has been minimized.

Copy link
@balloob

balloob Jul 10, 2019

Member

This will be so good 🎉

@@ -312,9 +311,6 @@ def do_setup(hass, hass_config, config):
setup_services(hass, hass_config, track_new_found_calendars,
calendar_service)

# Ensure component is loaded
setup_component(hass, 'calendar', config)

This comment has been minimized.

Copy link
@balloob

balloob Jul 10, 2019

Member

that's so weird

@balloob
Copy link
Member

left a comment

This looks great !

'end': None,
'location': None,
'description': None
}

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 11, 2019

Author Member

Is it good to make the breaking change and remove the clean up and keep the last event data in state attributes, even when the event has passed?

This comment has been minimized.

Copy link
@balloob

balloob Jul 11, 2019

Member

I think it's fine. I think eventually I would want to remove any entity related things from Calendar and just have a calendar specific Lovelace card and automation triggers. Because right now we are not able to deal with overlapping events for example.

@MartinHjelmare MartinHjelmare marked this pull request as ready for review Jul 11, 2019

@MartinHjelmare MartinHjelmare requested a review from home-assistant/core as a code owner Jul 11, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

With the 0.96 beta cut now, I think we can merge this so it will have some time in dev before release.

I'll swing back later today and merge if no one does it before then.

@balloob balloob merged commit 177f5a3 into home-assistant:dev Jul 11, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.19%)
Details
codecov/project 94.19% (target 90%)
Details
@Leatherface75

This comment has been minimized.

Copy link

commented Jul 11, 2019

With 0.96.0b0 google calendar doesn't work anymore just says "Invalid config".
Going back to 0.95.4 again it works fine again.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

This PR isn't part of the 0.96 beta.

@Leatherface75

This comment has been minimized.

Copy link

commented Jul 11, 2019

Ok but something is wrong with Google Calendar in 0.96.0b0
Works fine with 0.95.4 with same configuration.

@MartinHjelmare

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Please open an issue.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jul 11, 2019

@MartinHjelmare MartinHjelmare deleted the MartinHjelmare:clean-calendar-component branch Jul 11, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.