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

Add Awair sensor platform #18570

Merged
merged 13 commits into from
Nov 25, 2018
Merged

Add Awair sensor platform #18570

merged 13 commits into from
Nov 25, 2018

Conversation

ahayworth
Copy link
Contributor

@ahayworth ahayworth commented Nov 19, 2018

Description:

This PR adds the Awair platform, to support Awair air quality monitors. A full description of the new platform may be found here

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: awair
    access_token: ACCESS_TOKEN

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

@bachya
Copy link
Contributor

bachya commented Nov 19, 2018

The config helper addition should be moved into its own PR; when adding in tests, etc., both of these together is a bit much for a single PR.

@ahayworth
Copy link
Contributor Author

The config helper addition should be moved into its own PR; when adding in tests, etc., both of these together a bit much for a single PR.

@bachya - I'll submit that as a separate PR, but it's currently required for tests to pass. I'm not sure the best way to split that up without forcing these tests to fail (and thus confusing everyone involved).

@ahayworth
Copy link
Contributor Author

@bachya Separate PR for the MAC address config validation is here.

If we don't accept #18571, I'll push a change to this PR to use the existing regex validator. If we do merge it, then I'll re-base this PR after we've discussed it and decided whether or not to merge it.

@ahayworth
Copy link
Contributor Author

@bachya #18571 seemed like overkill to baloob, so I've closed it and re-factored this PR.

homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
@ahayworth
Copy link
Contributor Author

@MartinHjelmare - Thank you for the review! I especially appreciated the pointers towards platforms that were implemented according to current best practices. I learn best by example, and clearly I picked the wrong examples before.

I think I addressed all of your feedback, and I think the platform is in a much better shape. Would you mind taking another look when you have time?

homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
@ahayworth
Copy link
Contributor Author

@MartinHjelmare Okay - feedback addressed again. Let me know if there's anything else I can change to make it a better component. :)

This commit adds a sensor platform for Awair devices, by accessing
their beta API. Awair heavily rate-limits this API, so we throttle
updates based on the number of devices found. We also allow for the
user to bypass API device listing entirely, because the device list
endpoint is limited to only 6 calls per day. A crashing or restarting
server would quickly hit that limit.

This sensor platform uses the python_awair library (also written
as part of this PR), which is available for async usage.
It's true that this is generally not a great idea, but we really don't
want to crash here. If we can't set up the platform, logging it and
continuing is the right answer.
- Bump python_awair to 0.0.2, which has support for more granular exceptions
- Ensure we have python_awair available in test
- Raise PlatformNotReady if we can't set up Awair
- Make the 'Awair score' its own sensor, rather than exposing it other ways
- Set the platform up as polling, and set a sensible default
- Pass in throttling parameters to the underlying data class, rather
than use hacky global variable access to dynamically set the interval
- Switch to dict access for required variables
- Use pytest coroutines, set up components via async_setup_component,
  and test/modify/assert in generally better ways
- Commit test data as fixtures
- Don't force updates in test, instead modify time itself and let
  homeassistant update things "normally".
- Remove unneeded polling attribute
- Rename timestamp attribute to 'last_api_update', to better reflect
  that it is the timestamp of the last time the Awair API servers
  received data from this device.
- Use that attribute to flag the component as unavailable when data
  is stale. My own Awair device periodically goes offline and it really
  hardly indicates that at all.
- Dynamically set fixture timestamps to the test run utcnow() value,
  so that we don't have to worry about ancient timestamps in tests
  blowing up down the line.
- Don't assert on entities directly, for the most part. Find desired
  attributes in ... the attributes dict.
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
Honestly, it's just a lot easier to keep track of patches. Moreover,
the ones I seem to have missed are now caught, and tests seem to
consistently pass.

Also, switch test_throttle_async_update to manipulating time more
explicitly.
I very much need to set up a script to do this quickly w/o tox, because
running flake8 is not enough!
@ahayworth ahayworth closed this Nov 23, 2018
@ahayworth ahayworth reopened this Nov 23, 2018
@ghost ghost added in progress and removed in progress labels Nov 23, 2018
@ahayworth ahayworth closed this Nov 24, 2018
@ghost ghost removed the in progress label Nov 24, 2018
@ahayworth ahayworth reopened this Nov 24, 2018
@ghost ghost added the in progress label Nov 24, 2018
@ahayworth
Copy link
Contributor Author

TestStatisticsSensor.test_initialize_from_database_with_maxage fails periodically in CI, which is why I keep closing/re-opening this. I'm not worried about it because it seems to be a really new test

@MartinHjelmare
Copy link
Member

Yeah, that test is flaky. We can ignore it.

Copy link
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! Just some small comments left.

homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/awair.py Outdated Show resolved Hide resolved
Copy link
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.

Great! Can be merged when build passes.

@ahayworth ahayworth closed this Nov 25, 2018
@ahayworth ahayworth reopened this Nov 25, 2018
@ghost ghost added in progress and removed in progress labels Nov 25, 2018
@ahayworth
Copy link
Contributor Author

Great! Can be merged when build passes.

Excellent, thank you! :)

@MartinHjelmare MartinHjelmare merged commit eb6b6ed into home-assistant:dev Nov 25, 2018
@ghost ghost removed the in progress label Nov 25, 2018
@balloob balloob mentioned this pull request Dec 12, 2018
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