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

Add services to set/update and cancel Nest ETA #17836

Merged
merged 4 commits into from Nov 6, 2018

Conversation

Projects
None yet
7 participants
@schmittx
Contributor

schmittx commented Oct 27, 2018

Description:

  • Add service cancel_eta to cancel ETA
  • Add service set_eta to set/update ETA (break out from existing set_mode service)
  • Rename service set_mode to set_away_mode [breaking change]

I chose to break out the ETA setting/updating function as it's own service because I think it's more logical to have discrete services for setting mode, setting/updating ETA and cancelling ETA. Feedback is welcome though.

Related issue (if applicable): fixes #15371, #15818

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7163

Example entry for configuration.yaml (if applicable):

N/A

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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:

  • Tests have been added to verify that the new code works.
trip_id, eta_begin, eta_end)
structure.set_eta(trip_id, eta_begin, eta_end)
else:
_LOGGER.error("Invalid structure: %s",

This comment has been minimized.

@awarecan

awarecan Oct 27, 2018

Contributor

only log error when target structure doesn't have thermostats

else:
_LOGGER.error("Invalid structure %s",
_LOGGER.error("Invalid structure: %s",

This comment has been minimized.

@awarecan

awarecan Oct 27, 2018

Contributor

This should not log as error

_LOGGER.info("Cancelling ETA for trip: %s", trip_id)
structure.cancel_eta(trip_id)
else:
_LOGGER.error("Invalid structure: %s",

This comment has been minimized.

@awarecan

awarecan Oct 27, 2018

Contributor

only log error when target structure doesn't have thermostats

_LOGGER.error("Invalid structure: %s",
service.data[ATTR_STRUCTURE])
def set_mode(service):

This comment has been minimized.

@awarecan

awarecan Oct 27, 2018

Contributor

Is set_away_mode a better method name?

This comment has been minimized.

@schmittx

schmittx Oct 27, 2018

Contributor

I agree, I think set_away_mode is more intuitive.

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@schmittx schmittx referenced this pull request Oct 27, 2018

Merged

Add services to set/update and cancel Nest ETA #7163

2 of 2 tasks complete
@schmittx

This comment has been minimized.

Contributor

schmittx commented Oct 27, 2018

@awarecan Updated per your comments and some additional clean up, docs PR also added.

@awarecan awarecan removed the docs-missing label Oct 27, 2018

trip_id, eta_begin, eta_end)
structure.set_eta(trip_id, eta_begin, eta_end)
else:
_LOGGER.info("No thermostats found in structure: %s, unable "

This comment has been minimized.

@awarecan

awarecan Oct 27, 2018

Contributor

This log is still not right. This else clause included two cases, 1) structure is not the target, 2) structure does not have thermostats. I think we should have different logging for it. 1) should be a debug level, 2) should be a info level

This comment has been minimized.

@schmittx

schmittx Oct 27, 2018

Contributor

You're right, sorry about that.

I thought about this more, and I think a better approach is to first validate ATTR_STRUCTURE against all possible structures. Then we can produce a log if user enters a non-existent structure.

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@schmittx

This comment has been minimized.

Contributor

schmittx commented Oct 31, 2018

@awarecan Does anyone else need to review this? All good to go now?

@bogdanalexe90

This comment has been minimized.

bogdanalexe90 commented Nov 3, 2018

when will this be merged? really interested about canceling ETAs

schmittx added some commits Oct 26, 2018

@schmittx schmittx force-pushed the schmittx:nest-cancel-eta branch from c320a41 to 4e2c17d Nov 6, 2018

@balloob balloob merged commit 42fea4f into home-assistant:dev Nov 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 93.05%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Add services to set/update and cancel Nest ETA (home-assistant#17836)
* Add service to cancel ETA

* Update test requirements

* Change service name and update logging

* Reformat logging to verify structures

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@schmittx schmittx referenced this pull request Nov 30, 2018

Merged

Add description for Nest services #18839

0 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment