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 Open-Meteo integration #60379

Merged
merged 8 commits into from
Dec 6, 2021
Merged

Add Open-Meteo integration #60379

merged 8 commits into from
Dec 6, 2021

Conversation

frenck
Copy link
Member

@frenck frenck commented Nov 25, 2021

Proposed change

Add an initial step towards adding the Open-Meteo integration:

https://open-meteo.com/en

Open-Meteo offers free weather forecast APIs for open-source developers and non-commercial use. No API key is required.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

I interpret the dev docs to mean that a forecast is required for weather entities, although it isn't very explicit on this point. Otherwise this is just a group of sensors.

https://developers.home-assistant.io/docs/core/entity/weather/

@frenck
Copy link
Member Author

frenck commented Nov 30, 2021

Forecast in not required. I have left it out for the initial PR and kept everything as small as possible.

entity_id: state.name
for entity_id in self.hass.states.async_entity_ids(ZONE_DOMAIN)
if entity_id != ENTITY_ID_HOME
and (state := self.hass.states.get(entity_id)) is not None
Copy link
Member

Choose a reason for hiding this comment

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

No need to check this as you get the entity IDs from the state machine.

Copy link
Member Author

@frenck frenck Dec 4, 2021

Choose a reason for hiding this comment

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

Actually, I do, as the comprehension uses state.name, as the returned state can be None it needs to be checked (typing completeness).

@property
def condition(self) -> str | None:
"""Return the current condition."""
if not self.coordinator.data.current_weather:
Copy link
Member

Choose a reason for hiding this comment

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

Should this be implemented as the available property instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes/no, in the end, current_weather can return None from the API, thus needs to be checked as typing demands it :)

@frenck frenck marked this pull request as draft December 2, 2021 18:15
@frenck
Copy link
Member Author

frenck commented Dec 2, 2021

Marking the PR as draft, to ensure it keeps out of .12 (as I won't have time to complete it/extend it before beta starts).

Copy link
Contributor

@gwww gwww left a comment

Choose a reason for hiding this comment

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

Should consider the changes that this PR introduces around units: #59533

@frenck frenck marked this pull request as ready for review December 4, 2021 22:16
@balloob balloob merged commit d802f3a into dev Dec 6, 2021
@balloob balloob deleted the frenck-2021-3087 branch December 6, 2021 18:01
epenet added a commit to epenet/home-assistant-core that referenced this pull request Dec 6, 2021
epenet added a commit that referenced this pull request Dec 6, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2021
@emontnemery emontnemery restored the frenck-2021-3087 branch December 16, 2021 19:36
@frenck frenck deleted the frenck-2021-3087 branch March 10, 2022 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants