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

Raise PlatformNotReady for RMVtransport if API not available #17635

Merged
merged 7 commits into from Oct 26, 2018

Conversation

Projects
None yet
7 participants
@cgtobi
Contributor

cgtobi commented Oct 20, 2018

Description:

Raise PlatformNotReady for RMVtransport sensor if the API not available.

Checklist:

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

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

  • New dependencies are only imported inside functions that use them (example).
@@ -185,12 +194,13 @@ def __init__(self, session, station_id, destinations, direction, lines,
@Throttle(SCAN_INTERVAL)
async def async_update(self):
"""Update the connection data."""
from RMVtransport.rmvtransport import RMVtransportApiConnectionError

This comment has been minimized.

@houndci-bot

houndci-bot Oct 20, 2018

blank line contains whitespace

@cgtobi cgtobi changed the title from Raise PlatformNotReady if API not available to Raise PlatformNotReady for RMVtransport if API not available Oct 20, 2018

@cgarwood

This comment has been minimized.

Member

cgarwood commented Oct 20, 2018

Breaking change since direction: config was changed to directions:?

@@ -24,7 +25,7 @@
CONF_STATION = 'station'
CONF_DESTINATIONS = 'destinations'
CONF_DIRECTION = 'direction'
CONF_DIRECTIONS = 'directions'

This comment has been minimized.

@Kane610

Kane610 Oct 20, 2018

Contributor

Breaking change, is this really necessary?

@cgtobi

This comment has been minimized.

Contributor

cgtobi commented Oct 20, 2018

Thanks for the feedback. That was not intended. I'll revert that as soon as I get home.

try:
_data = await self.rmv.get_departures(self._station_id,
products=self._products,
directionId=self._direction,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 20, 2018

Member

Should this be removed?

This comment has been minimized.

@cgtobi

cgtobi Oct 20, 2018

Contributor

Ehm, no. Thanks for spotting that.

@@ -90,6 +91,13 @@
next_departure.get(CONF_MAX_JOURNEYS),
next_departure.get(CONF_NAME),
timeout))
for sensor in sensors:
await sensor.async_update()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 20, 2018

Member

We could use asyncio.wait.

tasks = [hass.async_create_task(sensor.async_update()) for sensor in sensors]
if tasks:
    await asyncio.wait(tasks)
if not all(sensor.data.departures for sensor in sensors):
    raise PlatformNotReady

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 20, 2018

Member

We gain efficiency by using asyncio.wait but on the other hand lose efficiency by finishing all I/O before checking the result. 🤷‍♂️

This comment has been minimized.

@cgtobi

cgtobi Oct 20, 2018

Contributor

That is neat.

This comment has been minimized.

@cgtobi

cgtobi Oct 20, 2018

Contributor

You win some, you loose some, huh?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 20, 2018

Member

I think either approach is fine.

cgtobi added some commits Oct 20, 2018

@cgtobi

This comment has been minimized.

Contributor

cgtobi commented Oct 26, 2018

Is this good enough to be merged? (I don't want to sound pushy) I'd have another PR ready on top of that but I don't want to mix things up too much.

@MartinHjelmare MartinHjelmare merged commit 9f146a3 into home-assistant:dev Oct 26, 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 increased (+0.0005%) to 93.605%
Details

@wafflebot wafflebot bot removed the in progress label Oct 26, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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