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

Conversation

Projects
None yet
5 participants
@ahayworth
Copy link
Contributor

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

@ahayworth ahayworth requested a review from home-assistant/core as a code owner Nov 19, 2018

@wafflebot wafflebot bot added the in progress label Nov 19, 2018

@ahayworth ahayworth referenced this pull request Nov 19, 2018

Merged

Add documentation for the Awair sensor platform #7579

0 of 2 tasks complete
@bachya

This comment has been minimized.

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 ahayworth force-pushed the ahayworth:awair branch from 611b427 to db86fed Nov 19, 2018

@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 19, 2018

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

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 19, 2018

@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 ahayworth force-pushed the ahayworth:awair branch from 7a5f0b3 to 6ce81b5 Nov 20, 2018

@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 20, 2018

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

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 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
@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 21, 2018

@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?

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 tests/components/sensor/test_awair.py Outdated
@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 22, 2018

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

ahayworth added some commits Nov 18, 2018

Awair Sensor Platform
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.
Disable pylint warning for broad try/catch
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.
Awair platform PR feedback
- 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
Awair PR feedback, volume 2
- 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.

@ahayworth ahayworth force-pushed the ahayworth:awair branch from 0ccdc8e to 9b95f07 Nov 22, 2018

@ahayworth ahayworth force-pushed the ahayworth:awair branch from 9b95f07 to 3f018dd Nov 22, 2018

Show resolved Hide resolved tests/components/sensor/test_awair.py Outdated
Show resolved Hide resolved tests/components/sensor/test_awair.py Outdated
Switch to using a context manager for timestream modification
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.

ahayworth added some commits Nov 23, 2018

Fix pydocstyle error
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

@wafflebot wafflebot bot added in progress and removed in progress labels Nov 23, 2018

@ahayworth ahayworth closed this Nov 24, 2018

@wafflebot wafflebot bot removed the in progress label Nov 24, 2018

@ahayworth ahayworth reopened this Nov 24, 2018

@wafflebot wafflebot bot added the in progress label Nov 24, 2018

@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 24, 2018

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

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 24, 2018

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

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good! Just some small comments left.

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

ahayworth added some commits Nov 24, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Great! Can be merged when build passes.

@ahayworth ahayworth closed this Nov 25, 2018

@ahayworth ahayworth reopened this Nov 25, 2018

@wafflebot wafflebot bot added in progress and removed in progress labels Nov 25, 2018

@ahayworth

This comment has been minimized.

Copy link
Contributor

ahayworth commented Nov 25, 2018

Great! Can be merged when build passes.

Excellent, thank you! :)

@MartinHjelmare MartinHjelmare merged commit eb6b6ed into home-assistant:dev Nov 25, 2018

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 increased (+0.002%) to 93.02%
Details

@wafflebot wafflebot bot removed the in progress label Nov 25, 2018

@balloob balloob referenced this pull request Dec 12, 2018

Merged

0.84 #19215

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