Adds the AirVisual air quality sensor platform by bachya · Pull Request #9320 · home-assistant/core

bachya · 2017-09-06T19:00:27Z

Description:

Adds a sensor platform for AirVisual air quality and pollution data.

Related issue (if applicable): N/A

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

Example entry for `configuration.yaml` (if applicable):

sensor:
  - platform: airvisual
    api_key: abc123
    monitored_conditions:
      - us
      - cn
    latitude: 42.81212
    longitude: 108.12422
    radius: 500

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:
- [ ] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
~~- [ ] Tests have been added to verify that the new code works.~~

houndci-bot · 2017-09-06T19:50:25Z

+    @property
+    def pollution_info(self):
+        """Define a property to access the pollution information."""
+        return self._pollution_info # pylint: disable=syntax-error


at least two spaces before inline comment

MartinHjelmare

In general looks very good I think. Some comments below.

MartinHjelmare · 2017-09-07T16:57:20Z

@@ -0,0 +1,304 @@
+"""Define sensors for AirVisual air quality information."""


Look at another sensor platform (eg DHT) for our default way of writing the module docstring. It should contain a link to the docs.

MartinHjelmare · 2017-09-07T17:03:18Z

+ATTR_COUNTRY = 'country'
+ATTR_POLLUTANT_SYMBOL = 'pollutant_symbol'
+ATTR_POLLUTANT_UNIT = 'pollutant_unit'
+ATTR_STATE = 'state'


Import from const.py.

I was under the impression that const.py was for generally-useful constants, and that attributes applying to certain platforms only should exist in those platform files. I could see country and state being generally-useful, but what about the two pollutant ones?

You're correct, you shouldn't add non general constants to const.py. I'm only talking about ATTR_STATE which already exists in const.py.

Ah, got it! Will do!

MartinHjelmare · 2017-09-07T17:21:41Z

+    @property
+    def state(self):
+        """Define a property to access the state."""
+        return self._state


Consider making all these properties regular instance attributes instead, since you don't seem to need any more logic than returning the attribute.

MartinHjelmare · 2017-09-07T17:24:24Z

+        if self._data:
+            return merge_two_dicts(super().device_state_attributes, {
+                ATTR_POLLUTANT_SYMBOL: self._symbol,
+                ATTR_POLLUTANT_UNIT: self._unit


Don't you want to have a unit of measure for these sensors? Why not overwrite the unit_of_measurement property?

I see you're working on that now. 😉

Would love your thoughts here:

The Air Quality Index doesn't really have a unit; I want to add something so I get a nice graph, but not sure what it would be.

The Air Quality Level doesn't have a unit, since its values are strings like "Good".

Same for the Main Pollutant.

Maybe use 'PSI' as unit for the pollution index?

PSI isn't quite right, as we're not specifically measuring pressure. Sadly, the EPA specifically states that AQI:

Ranges from 0 to 500 (no units)

Check out my latest push; not sure if you'll approve, but if I return an empty string for that sensor's unit_of_measurement property, I get the bar chart that I want and the unit_of_measurement attribute is the technically correct value: nothing.

I was referring to pollutant standard index:
https://en.m.wikipedia.org/wiki/Pollutant_Standards_Index

An empty string could be OK, if we make that standard for all unit less measurements that should have a line chart.

* Adds the AirVisual air quality sensor platform * Updated .coveragerc * Removed some un-needed code * Adding strangely-necessary pylint disable * Removing a Python3.5-specific dict combiner method * Restarting stuck coverage test * Added units to AQI sensor (to get nice graph) * Making collaborator-requested changes * Removing unnecessary parameter from data object

bachya added 2 commits September 6, 2017 12:50

Adds the AirVisual air quality sensor platform

6f142ab

Updated .coveragerc

4f0062e

homeassistant added new-platform platform: sensor.airvisual cla-signed labels Sep 6, 2017

bachya mentioned this pull request Sep 6, 2017

Airvisual home-assistant/home-assistant.io#3320

Closed

Removed some un-needed code

26df7f4

bachya mentioned this pull request Sep 6, 2017

Adds documentation for the proposed AirVisual sensor platform home-assistant/home-assistant.io#3321

Merged

Adding strangely-necessary pylint disable

fed4722

houndci-bot reviewed Sep 6, 2017

View reviewed changes

bachya added 3 commits September 6, 2017 13:54

Removing a Python3.5-specific dict combiner method

d012e7b

Restarting stuck coverage test

00b3453

Added units to AQI sensor (to get nice graph)

06a2e9f

MartinHjelmare requested changes Sep 7, 2017

View reviewed changes

bachya added 2 commits September 7, 2017 18:12

Making collaborator-requested changes

0bacae6

Removing unnecessary parameter from data object

cd53c57

MartinHjelmare approved these changes Sep 8, 2017

View reviewed changes

MartinHjelmare added this to the 0.53 milestone Sep 8, 2017

MartinHjelmare merged commit d1ef473 into home-assistant:dev Sep 8, 2017

bachya deleted the airvisual branch September 8, 2017 15:01

balloob mentioned this pull request Sep 9, 2017

0.53 #9327

Merged

balloobbot added the cherry-picked label Sep 9, 2017

home-assistant locked and limited conversation to collaborators Dec 11, 2017

ghost added integration: airvisual and removed platform: sensor.airvisual labels Mar 21, 2019

		@@ -0,0 +1,304 @@
		"""Define sensors for AirVisual air quality information."""

Uh oh!

Conversation

bachya commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bachya Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bachya commented Sep 6, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

bachya Sep 8, 2017 •

edited

Loading