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 GIOS integration #28719

Merged
merged 25 commits into from Dec 31, 2019
Merged

Add GIOS integration #28719

merged 25 commits into from Dec 31, 2019

Conversation

bieniu
Copy link
Member

@bieniu bieniu commented Nov 12, 2019

Breaking Change:

None

Description:

New integration that shows air quality data in Poland from GIOŚ (Polish Chief Inspectorate Of Environmental Protection) service http://powietrze.gios.gov.pl/pjp/current

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

Example entry for configuration.yaml (if applicable):

Integration uses only config_flow configuration.

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.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Copy link
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very brief review.

@property
def icon(self):
"""Return the icon."""
if self._aqi == "bardzo dobry":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be defined as constants (better even if defined in the backend library) in english.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GIOS server sends AQI states in Polish. Do you think I need to translate those to English in backend library?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no other identifier besides the (translated) string? Imo it would make sense to have those as constants as it is just cleaner that way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AQI state strings from server are only in Polish. I added consts with English names for that states. I think that it's now understandable for non-Polish people.

return self._aqi

@property
@round_state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should rounding be left to the consumers of the data? I don't personally mind, but I think someone might :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rytilahti In Poland, most air quality services on the Internet rounding off values. I think this is a good approach.

homeassistant/components/gios/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/gios/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/gios/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/gios/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/gios/air_quality.py Outdated Show resolved Hide resolved
homeassistant/components/gios/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/gios/config_flow.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Dec 18, 2019
Dev automation moved this from Review in progress to Reviewer approved Dec 18, 2019
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!


if user_input is not None:
try:
already_configured = self._async_current_ids()
Copy link
Member

@Kane610 Kane610 Dec 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set unique ID first and then you can use self._abort_if_unique_id_configured() here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@Kane610 Kane610 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!

@MartinHjelmare MartinHjelmare merged commit 2c1a7a5 into home-assistant:dev Dec 31, 2019
Dev automation moved this from Reviewer approved to Done Dec 31, 2019
@lock lock bot locked and limited conversation to collaborators Jan 1, 2020
@bieniu bieniu deleted the gios branch January 8, 2020 14:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants