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

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

Merged
merged 6 commits into from Feb 2, 2019

Conversation

Projects
None yet
5 participants
@rohankapoorcom
Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Member

dgomes commented Jan 23, 2019

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

@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 25, 2019

@balloob is something like that allowed?

@frenck frenck added the docs-missing label Jan 25, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

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

Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/fastdotcom/sensor.py Outdated
@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 28, 2019

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

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

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

Show resolved Hide resolved homeassistant/components/fastdotcom/sensor.py
Show resolved Hide resolved homeassistant/components/fastdotcom/sensor.py Outdated

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:fastdotcom branch from 0148432 to 05f8421 Feb 1, 2019

@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Feb 1, 2019

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

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 1, 2019

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

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Feb 2, 2019

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

@rohankapoorcom rohankapoorcom merged commit fcccf13 into home-assistant:dev Feb 2, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.044%
Details

@rohankapoorcom rohankapoorcom deleted the rohankapoorcom:fastdotcom branch Feb 2, 2019

@wafflebot wafflebot bot removed the in progress label Feb 2, 2019

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.