Skip to content

Conversation

@brg468
Copy link
Contributor

@brg468 brg468 commented Mar 30, 2020

Proposed change

This PR adds Rachio fixed schedules as switches. This allows an entire schedule to be started or stopped from HomeAssistant, instead of only individual zones for a pre-determined amount of time.

This is my first PR so go easy on me!

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@homeassistant
Copy link
Contributor

Hi @brg468,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @bdraco, mind taking a look at this pull request as its been labeled with a integration (rachio) you are listed as a codeowner for? Thanks!

@bdraco
Copy link
Member

bdraco commented Mar 30, 2020

@brg468 I have a PR coming up to cleanup some of the duplicate code #33422. It looks like the only conflict will be the constants.

@bdraco
Copy link
Member

bdraco commented Mar 30, 2020

@brg468 Nice work. I'll review this after #33422 goes in as it should make this a bit smaller

@brg468
Copy link
Contributor Author

brg468 commented Mar 30, 2020

Sounds good

@bdraco
Copy link
Member

bdraco commented Mar 31, 2020

@brg468 This should be ready for rebase.
Let me know if you have trouble with the conflicts

https://developers.home-assistant.io/docs/dev

@brg468
Copy link
Contributor Author

brg468 commented Apr 1, 2020

@bdraco That took me way longer than it should have but I think its finally ready.. I just noticed @MartinHjelmare's comments and noticed at least 1 applies to this. Is it easier if I fix that now?

@bdraco
Copy link
Member

bdraco commented Apr 1, 2020

@bdraco That took me way longer than it should have but I think its finally ready.. I just noticed @MartinHjelmare's comments and noticed at least 1 applies to this. Is it easier if I fix that now?

Probably best for me to do it in a followup so we don't expand the scope anymore and can get this one merged

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Looks like just the missing async_will_remove_from_hass and the __str__ removal left.

@bdraco
Copy link
Member

bdraco commented Apr 1, 2020

@brg468 I tried this with 6 different systems. The schedule appears for some but not others.

I'm looking into it now

@brg468
Copy link
Contributor Author

brg468 commented Apr 1, 2020

@brg468 I tried this with 6 different systems. The schedule appears for some but not others.

I'm looking into it now

Just to make sure we're on the same page, these are fixed schedules that aren't showing up, not flex schedules correct?

@bdraco
Copy link
Member

bdraco commented Apr 1, 2020

@brg468 I tried this with 6 different systems. The schedule appears for some but not others.
I'm looking into it now

Just to make sure we're on the same page, these are fixed schedules that aren't showing up, not flex schedules correct?

Just looked at the raw data, they are flexScheduleRules

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

@brg468
Copy link
Contributor Author

brg468 commented Apr 1, 2020

@brg468 I tried this with 6 different systems. The schedule appears for some but not others.
I'm looking into it now

Just to make sure we're on the same page, these are fixed schedules that aren't showing up, not flex schedules correct?

Just looked at the raw data, they are flexScheduleRules

I never tried it before because looking at the dependency I assumed it wouldn't work, but you can actually trigger a flex schedule in the same manner. That can be a project for another day..

@bdraco
Copy link
Member

bdraco commented Apr 1, 2020

I never tried it before because looking at the dependency I assumed it wouldn't work, but you can actually trigger a flex schedule in the same manner. That can be a project for another day..

Nice, new PR for sure though to keep scope reasonable

_LOGGER.debug("Rachio setting up zone: %s", zone)
entities.append(RachioZone(person, controller, zone, current_schedule))
for sched in schedules:
_LOGGER.debug("Added schedule: %s", sched)
Copy link
Member

Choose a reason for hiding this comment

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

If it's still enough info, it would save on the logging calls if we just logged the list with added entities once. The entity has a repr magic method and will print the name and current state.

self._undo_dispatcher = None

@property
def schedule_id(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Generally we don't need to define properties if we just return the instance attribute. We could instead just make the instance attribute public and name it appropriately.

self._current_schedule = self._controller.current_schedule
return self.schedule_id == self._current_schedule.get(KEY_SCHEDULE_ID)

def _handle_update(self, *args, **kwargs) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to run in the executor thread pool? If not, we should decorate it with @callback imported from core.py and prefix the name with async_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to add another PR that allows all schedules to be used instead of just fixed schedules. I'll address these changes in that PR.

@brg468 brg468 mentioned this pull request Apr 2, 2020
20 tasks
@lock lock bot locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants