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

Refactor Waze Travel Time #21632

Closed
wants to merge 8 commits into from
Closed

Refactor Waze Travel Time #21632

wants to merge 8 commits into from

Conversation

Petro31
Copy link
Contributor

@Petro31 Petro31 commented Mar 3, 2019

Refactored Waze Travel Time to contain a data object.

  • Changed error retrieving data to a warning.
  • Added distance conversion depending on region.
  • Removed dependency on TRACKABLE_DOMAINS list.
  • Update to use WazeRouteCalculator 0.10

Description:

Related issue (if applicable): fixes #

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

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.

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.

Refactored Waze Travel Time to contain a data object.

* Changed error retrieving data to a warning.
* Added distance conversion depending on region.
* Removed dependency on TRACKABLE_DOMAINS list.
Petro31 added a commit to Petro31/home-assistant.io that referenced this pull request Mar 5, 2019
@ljmerza
Copy link
Contributor

ljmerza commented Mar 7, 2019

Wanted to mention this PR #21153 gets rid of update for google_time_travel so probably be best to do that with waze as well. There's a discussion of getting rid of update on sensors home-assistant/architecture#131

@Petro31
Copy link
Contributor Author

Petro31 commented Mar 7, 2019

Wanted to mention this PR #21153 gets rid of update for google_time_travel so probably be best to do that with waze as well. There's a discussion of getting rid of update on sensors home-assistant/architecture#131

There is no update service for this component... nothing to remove.

@ljmerza
Copy link
Contributor

ljmerza commented Mar 7, 2019

how do the waze values get continuously updated without something triggering it?

I see hass.bus.listen_once(EVENT_HOMEASSISTANT_START, lambda _: sensor.update()) does this mean waze only gets updated on boot and that's it?

@Petro31
Copy link
Contributor Author

Petro31 commented Mar 7, 2019

how do the waze values get continuously updated without something triggering it?

I see hass.bus.listen_once(EVENT_HOMEASSISTANT_START, lambda _: sensor.update()) does this mean waze only gets updated on boot and that's it?

It gets updated through the update method. Like every other sensor. I believe you are confusing the update service with the update method.

To reenforce my reasoning, here is the expert that was removed from google_travel_time that is exactly what is being talked about.

        if DATA_KEY not in hass.data:	
            hass.data[DATA_KEY] = []	
            hass.services.register(	
                DOMAIN, 'google_travel_sensor_update', update)

Registering for a service. Not the fact that the component has an update method.

@ljmerza
Copy link
Contributor

ljmerza commented Mar 7, 2019

hey fyi someone is having an issue with UTF-8 decoding #21739

@mikeage
Copy link
Contributor

mikeage commented Mar 7, 2019

It's not surprising, given the change, but I just took this PR locally, and confirmed the the UTF issue (really ISO-8859-1 decoding, I think) still fails.

@mikeage
Copy link
Contributor

mikeage commented Mar 7, 2019

FWIW, replacing: self.route = bytes(route, 'ISO-8859-1').decode('UTF-8') with just self.route = route seems to work. I'm not terribly familiar with how python3 handles unicode, or why this conversion existed in the first place, so I'm not sure if this is right. But it did work for my routes :-)

edit: this seems like it was introduced in the last change to waze, which upgraded the wazeroutecalculator to a newer version that natively returns utf-8.

Added vehicle type, removed decode statement.
@Petro31
Copy link
Contributor Author

Petro31 commented Mar 10, 2019

FYI we may want to hold off on this PR until WazeRouteCalculator is fixed. There is a serious bug that affects Home Assistant users. The PR for it is: kovacsbalu/WazeRouteCalculator#37

@ljmerza ljmerza mentioned this pull request Mar 14, 2019
4 tasks
@Petro31 Petro31 mentioned this pull request Mar 14, 2019
@Petro31
Copy link
Contributor Author

Petro31 commented Mar 18, 2019

@pvizeli I'm unable to resolve merge conflicts due to access rights. This PR is ready in regards to WazeRouteCalculator updates.

@Petro31 Petro31 closed this Mar 20, 2019
@ghost ghost removed the in progress label Mar 20, 2019
balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants