Skip to content

Split out fastdotcom into a component and a sensor platform#20341

Merged
rohankapoorcom merged 6 commits into
home-assistant:devfrom
rohankapoorcom:fastdotcom
Feb 2, 2019
Merged

Split out fastdotcom into a component and a sensor platform#20341
rohankapoorcom merged 6 commits into
home-assistant:devfrom
rohankapoorcom:fastdotcom

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Jan 23, 2019

Description:

Per home-assistant/architecture#131, sensor platforms should not be registering services. Fast.com is one of several that does and this should be cleaned up. This sensor has a service call which is used to manually run a speedtest and them update the sensor with that information.

I moved all of the setup/service logic to a new Fast.com component and then used that component
inside the sensor platform. All configuration has been moved to the component and the sensor platform is loaded automatically when the component is loaded.

Using a dispatcher signal, the component notifies the platform when it's finished a speedtest so the platform can update it's data.

Breaking Change: The Fast.com sensor platform has been separated into a component and a sensor to remove the service from the sensor platform. Configuration options have changed, please review the documentation and select the correct options.

Tests will not pass until #20526 is merged.

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

Example entry for configuration.yaml (if applicable):

fastdotcom:

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.

@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Jan 23, 2019

As pointed out in the architecture thread the update service in Fast.com (and the other 3 sensors) is redundant, since we have since the homeassistant.update_entity service which can be used to trigger the speedtest. It would just be a matter of removing the service from the sensors altogether.

On the other hand, if you really want to move the sensor into a component, might as well add the config_flow :)

@rohankapoorcom
Copy link
Copy Markdown
Member Author

I couldn't find a way to make that work for this component without running the speed test more frequently (whenever Home Assistant calls the entity update). By letting it run directly in the update method, it's going to use a lot more bandwidth.

The way the component is designed, it should only run speed tests at the frequency specified in the config and then on demand, if required.

Is there some way I'm missing that will allow that to work without a separate service?

@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Jan 23, 2019

You could probably use a configurable SCAN_INTERVAL (don't know if that's allowed)

@rohankapoorcom
Copy link
Copy Markdown
Member Author

@balloob is something like that allowed?

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

I think it's ok to have a component + sensor platform like this. The centralized api access is good motivation.

Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/sensor.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Comment thread homeassistant/components/fastdotcom/sensor.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member Author

Note: Tests will not pass until #20526 is merged (I centralized a const rather than redefining it).

Comment thread homeassistant/components/fastdotcom/__init__.py
Comment thread homeassistant/components/fastdotcom/__init__.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Also add should_poll property in the sensor and set it to False.

Comment thread homeassistant/components/fastdotcom/sensor.py
Comment thread homeassistant/components/fastdotcom/sensor.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member Author

Rebased and pushed up, tests should now pass. @MartinHjelmare I believe all of your comments were addressed.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@MartinHjelmare
Copy link
Copy Markdown
Member

Can be merged when docs PR is linked in the PR description, a paragraph is added in the PR description about the breaking change for the release notes, and the build passes.

@rohankapoorcom
Copy link
Copy Markdown
Member Author

Docs linked, breaking change PR added, build passing. Merging.

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.

5 participants