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

Added Waze travel time sensor #12387

Merged
merged 2 commits into from Mar 31, 2018

Conversation

Projects
None yet
5 participants
@Myrddyn1
Contributor

Myrddyn1 commented Feb 13, 2018

Description:

Sensor fetching routes from the script WazeRouteCalculator.

Documentation:

Documentation pull request:
home-assistant/home-assistant.io#4498

Example entry for configuration.yaml:

sensor:
  - platform: waze_travel_time
    name: "Example Name"
    origin: Montréal, QC
    destination: Québec, QC
    region: 'US'
    update_interval: '00:03'
    outputs:
     - duration
     - distance
     - route
@fabaff

This comment has been minimized.

Member

fabaff commented Feb 13, 2018

Please push your changes to the existing branch instead of open a new Pull Request. It's pretty hard to follow the changes.

Successor of #11870 and #12386.

vol.All(cv.ensure_list, [vol.In(OUTPUT_TYPES)]),
vol.Optional(CONF_REGION, default='US'):
vol.All(cv.ensure_list, [vol.In(REGIONS)]),
vol.Optional(CONF_UPDATE_INTERVAL, default=timedelta(seconds=300)):

This comment has been minimized.

@balloob

balloob Mar 1, 2018

Member

Don't specify an CONF_UPDATE_INTERVAL. Just specify a SCAN_INTERVAL as constant and Home Assistant will pick it up and allow users to override it via config: https://home-assistant.io/docs/configuration/platform_options/#scan-interval

@balloob

One minor change. For the rest looks great ! 👍

name = config.get(CONF_NAME)
origin = config.get(CONF_ORIGIN)
destination = config.get(CONF_DESTINATION)
region = config.get(CONF_REGION)[0]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Why allow a list of regions in the config if we're only interested in the first item?

)
except requests.exceptions.HTTPError as error:
_LOGGER.error("Error: %s", error)
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Nothing is checking this return value. Just returnhere.

"""Icon to use in the frontend, if any."""
return self._icon
def __update(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Name this update.

_LOGGER.error("Error on retreiving data: %s", exp)
return
def __update(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Name this update.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Annotate the method with @Throttle(...)

self._region = region
self.data = {}
self.update = Throttle(update_interval)(self.__update)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

Use the throttle as a decorator instead.

self._resolution = OUTPUT_TYPES[output_type][3]
self.waze_data = waze_data
self._state = None
self.update = Throttle(update_interval)(self.__update)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Mar 2, 2018

Member

We don't need the throttle here as we already throttle the data class update.

@fabaff fabaff dismissed stale reviews from balloob and MartinHjelmare Mar 31, 2018

Comments addressed

@fabaff

fabaff approved these changes Mar 31, 2018

@fabaff fabaff merged commit 7bf8d4a into home-assistant:dev Mar 31, 2018

5 checks passed

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.3%) to 93.754%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Apr 13, 2018

Merged

0.67.0 #13856

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.