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 nilu air_quality platform #19674

Merged
merged 7 commits into from Jan 27, 2019

Conversation

Projects
None yet
4 participants
@hfurubotten
Copy link
Contributor

hfurubotten commented Dec 30, 2018

Description:

New platform in the air_quality platform. Shows data from the governmental air research institute sensor stations within Norway.

Rewritten from a sensor platfrom to air_quality platform. Original PR here: #18104

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

Example entry for configuration.yaml (if applicable):

air_quality:
  - platform: nilu
    show_on_map: True
  - platform: nilu
    show_on_map: True
    area: Bergen

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.
Show resolved Hide resolved homeassistant/components/air_pollutants/nilu.py Outdated
Show resolved Hide resolved homeassistant/components/air_pollutants/nilu.py Outdated
Show resolved Hide resolved homeassistant/components/air_pollutants/nilu.py Outdated
Show resolved Hide resolved homeassistant/components/air_pollutants/nilu.py Outdated
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 2, 2019

Let's wait here until we know if #19448 should be merged first.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 5, 2019

#19448 has been merged so we need to rebase and move this platform under the air_quality component.

hfurubotten added some commits Dec 30, 2018

Code Review - validation, DRYs, rm state override, new attr
- Repeated code moved to own method.
- Removed override of state property.
- New attr for showing nilu pollution recommendations.
- More validation of stations input.
- Minor fixes and typos.

@hfurubotten hfurubotten force-pushed the hfurubotten:air-pollutants-nilu branch from 1223c49 to 1c1a9a7 Jan 6, 2019

@hfurubotten hfurubotten changed the title Add nilu air_pollutants platform Add nilu air_quality platform Jan 6, 2019

@hfurubotten

This comment has been minimized.

Copy link
Contributor Author

hfurubotten commented Jan 6, 2019

It should be rebased and moved to the air_quality component now.
Please check that everything is as it should. Got myself into trouble when rebasing, but I think I have gotten everything into order. However a double check would be nice.

Show resolved Hide resolved homeassistant/components/air_quality/nilu.py Outdated
Show resolved Hide resolved homeassistant/components/air_quality/nilu.py Outdated
if component_name in self._api.data.sensors:
sensor = self._api.data.sensors[component_name]
return "{0:.2f} {1}".format(sensor.value,
sensor.unit_of_measurement)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 7, 2019

Member

There should preferably not be a unit of measurement but that should be standardized. Maybe we haven't done that yet? @fabaff

This comment has been minimized.

@hfurubotten

hfurubotten Jan 8, 2019

Author Contributor

Is it possible to add a property where it gives the unit of measurement, such as on sensors? I was not sure if all measured components should be defined in µg/m³, so I added it to the output of each.

All measured pollutants currently supported from the NILU-client is measured in µg/m³ or mg/m³, so one fit it all is possible with what the air_quality component gives of information now. CO/carbon monoxide is the one thats measured in mg/m³, but this can easily be converted to µg/m³ to get a one fits it all.

Can add this to the platform:

    @property
    def unit_of_measurement(self) -> str:
        """Return the unit this state is expressed in."""
        return 'µg/m³'

This comment has been minimized.

@fabaff

fabaff Jan 27, 2019

Member

There should be no unit of measurements because the frontend should take care about this.

This comment has been minimized.

@hfurubotten

hfurubotten Jan 27, 2019

Author Contributor

I will fix the output. What is the default unit the frontend expect, so I can make sure they are correct or converted?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 8, 2019

Besides the property units this looks good! I'd like @fabaff to comment about that, if there's a plan/spec for that, before we merge.

@fabaff fabaff merged commit 7412d0f into home-assistant:dev Jan 27, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 92.992%
Details

@wafflebot wafflebot bot removed the in progress label Jan 27, 2019

@hfurubotten hfurubotten deleted the hfurubotten:air-pollutants-nilu branch Jan 28, 2019

fredrike added a commit to fredrike/home-assistant that referenced this pull request Jan 30, 2019

Add nilu air_quality platform (home-assistant#19674)
* Add nilu air_pollutants platform

* Code Review - validation, DRYs, rm state override, new attr
- Repeated code moved to own method.
- Removed override of state property.
- New attr for showing nilu pollution recommendations.
- More validation of stations input.
- Minor fixes and typos.

* Removed unused prop

* Check for none result from client before entity add

* Moved platform to air_quality component

* Updated outdated docstrings

* Minor changes

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

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