Met.no migrate from classic to complete endpoint #39493

thimic · 2020-08-31T07:46:26Z

Breaking change

In updating pyMetno to use the newer endpoint, some of the calculations and forecast aggregations were tweaked a bit:

Use hourly forecast for current weather, not daily
Ensure compared datetime objects are compared in the same time zone
Use highest resolution data from full 24 hours to calculate daily forecast min/max/sum values

None of these changes are expected to break someone's setup, though the data presented might look a little different due to the above.

In addition, all time stamps are now given in UTC. Automations that depend on the datetime key under the state attribute forecast needs to be checked and updated.

Proposed change

Bump met component requirement to myMetno-0.8.1: https://github.com/Danielhiversen/pyMetno/releases/tag/0.8.1

The new release has the following changes.

Update code to handle new endpoint json format: https://api.met.no/weatherapi/locationforecast/2.0/complete
Add precipitation_probability to forecast parameters

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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:

Documentation added/updated for www.home-assistant.io

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.
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:

I have reviewed two other open pull requests in this repository.

- Update code to handle new endpoint json format - Use hourly forecast for current weather, not daily - Update precipitation calculation - Ensure compared datetime objects are compared in the same time zone - Use data from full 24 hours to calculate daily forecast min/max/sum values - Add `precipitation_probability` to forecast parameters

homeassistant · 2020-08-31T07:46:28Z

Hi @thimic,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

probot-home-assistant · 2020-08-31T07:47:27Z

Hey there @Danielhiversen, mind taking a look at this pull request as its been labeled with an integration (met) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

Danielhiversen

👍

homeassistant/components/met/__init__.py

frenck · 2020-08-31T10:05:56Z

Thank you for your contribution thus far! 🎖 Since this is a significant contribution (counting the upstream changes as well), we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

thimic · 2020-08-31T10:18:14Z

Hi @frenck,

Code owners have been updated.

I see there are a few failing checks on the CI. I'm having a bit of an issue getting my local dev environment to run tests and it's getting late where I am. Might have to come back to this tomorrow.

MatthewFlamm · 2020-08-31T12:31:06Z

With the addition of precipitation_probability, does it make more sense to have those constants use the constants in home assistant? Right now, if any of those constants change in home assistant, it requires pymetno to change as well. Yet homeassistant is not a dependency of pymetno.

MartinHjelmare

The weather forecast data returned in the weather forecast property must be defined within home assistant as it's part of the home assistant API. Changing the library must not change the returned data implicitly. Changes should be explicitly done inside home assistant.

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

thimic · 2020-08-31T20:20:12Z

So the change would be to have pyMetno return keys native to it, then map those keys to HA constants in the HA met.no component?

MartinHjelmare · 2020-08-31T20:28:34Z

Yes, exactly. 👍

thimic · 2020-08-31T20:55:22Z

Cool, happy to make that change.

@Danielhiversen, I see two ways forward:

Keep pyMetno unchanged and update the HA met.no component with a map from HA constants to pyMetno keys. Basically a 1:1 map as of today.
Change pyMetno to return key names directly from the met.no API, then map these new keys in the HA met.no component.

The latter has the benefit of pyMetno returning more accurate forecasts for direct users of pyMetno, with the drawback that it'll be a breaking change for those very users.

The former is less work and less hassle for users upgrading, but leaves an unstated connection between pyMetno and the HA component from the pyMetno side.

thimic · 2020-09-01T11:31:16Z

The component is now using constants. I also noticed met.no serves wind direction in degrees, whereas Home Assistant expects a 1-3 letter string according to the docs.

It's all in need of a proper test. I've so far been lazy and testing on my production instance, though I'm unable to do that currently after a recent change to the dev branch. Will set up a proper dev environment tomorrow and test there.

MartinHjelmare · 2020-09-01T14:37:15Z

The frontend should be able to convert azimuth angle degrees to cardinal direction letters, so we should keep the current state to not break user automations. We allow both ways at the moment.

I have a PR open to update the docs.

homeassistant/components/weather/__init__.py

homeassistant/components/met/weather.py

* Roll back conversion of wind direction to cardinal direction letters * Remove ATTR_WEATHER_CONDITION constant from weather component * Filter out None value keys from forecast

MartinHjelmare · 2020-09-02T09:01:45Z

Looks good! Seems there's a test failure for the met integration.

thimic · 2020-09-02T09:07:34Z

Working on that now. Getting set up with a proper dev environment so I can run tests locally.

thimic · 2020-09-02T09:53:36Z

This test fails for me on the dev branch without this update as well. Might be related to b258e75. I'll poke around some more.

…ata()` This causes a test to fail in home-assistant/core#39493

thimic · 2020-09-02T10:47:31Z

Okay, the test is failing due to an issue I introduced in pyMetno 0.8.0. I've created a PR for that: Danielhiversen/pyMetno#25

thimic · 2020-09-02T10:53:48Z

On a side note, pyMetno 0.8.0 and later serves time stamps in UTC rather than local time zones with an offset as described in #38193.

Let me know if you'd prefer not rocking the boat and reverting that change.

…ata()` (#25) This causes a test to fail in home-assistant/core#39493

MartinHjelmare · 2020-09-02T11:03:30Z

UTC is the correct time format for state attributes. That's our standard. 👍

Please mention it in the breaking changes.

MartinHjelmare · 2020-09-02T11:06:08Z

Ping me when the description is updated:

Breaking changes.
Release notes link for the version bump.

Then we can merge.

thimic · 2020-09-02T11:26:17Z

@MartinHjelmare I've made those updates. Black failed now, but it's on the vizio component that hasn't been touched in this PR.

MartinHjelmare

Great!

MartinHjelmare · 2020-09-02T12:10:59Z

Black will be fixed here:
#39573

homeassistant added the cla-needed label Aug 31, 2020

probot-home-assistant bot added dependency integration: met small-pr PRs with less than 30 lines. labels Aug 31, 2020

probot-home-assistant bot assigned Danielhiversen Aug 31, 2020

homeassistant added cla-signed and removed cla-needed labels Aug 31, 2020

frenck changed the title ~~Migrate from classic to complete endpoint~~ Met.no migrate from classic to complete endpoint Aug 31, 2020

Fix requirements

b44febf

Danielhiversen approved these changes Aug 31, 2020

View reviewed changes

ludeeus requested changes Aug 31, 2020

View reviewed changes

homeassistant/components/met/__init__.py Outdated Show resolved Hide resolved

ludeeus added the breaking-change label Aug 31, 2020

Use default URL

2d5a3de

Update code owners

5499714

thimic force-pushed the metno_api_v2_complete_endpoint branch from dc6ae00 to 5499714 Compare August 31, 2020 10:21

Satisfy isort

1eddb02

MartinHjelmare requested changes Aug 31, 2020

View reviewed changes

Use HA constants, calculate wind bearing code from angle

28bcb10

thimic force-pushed the metno_api_v2_complete_endpoint branch from b6657eb to 28bcb10 Compare September 1, 2020 11:44

MartinHjelmare reviewed Sep 1, 2020

View reviewed changes

homeassistant/components/weather/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/met/weather.py Outdated Show resolved Hide resolved

homeassistant/components/met/weather.py Outdated Show resolved Hide resolved

Address notes

982a81f

* Roll back conversion of wind direction to cardinal direction letters * Remove ATTR_WEATHER_CONDITION constant from weather component * Filter out None value keys from forecast

thimic added a commit to thimic/pyMetno that referenced this pull request Sep 2, 2020

Fix exception if get_current_weather() is called before `fetching_d…

1426a1e

…ata()` This causes a test to fail in home-assistant/core#39493

thimic added a commit to thimic/pyMetno that referenced this pull request Sep 2, 2020

Fix exception if get_current_weather() is called before `fetching_d…

dad36a0

…ata()` This causes a test to fail in home-assistant/core#39493

thimic mentioned this pull request Sep 2, 2020

Fix get_current_weather() exception Danielhiversen/pyMetno#25

Merged

Danielhiversen pushed a commit to Danielhiversen/pyMetno that referenced this pull request Sep 2, 2020

Fix exception if get_current_weather() is called before `fetching_d…

b16b72e

…ata()` (#25) This causes a test to fail in home-assistant/core#39493

Bump pyMetno requirement to 0.8.1

f6334ef

MartinHjelmare added the waiting-for-upstream We're waiting for a change upstream label Sep 2, 2020

MartinHjelmare approved these changes Sep 2, 2020

View reviewed changes

MartinHjelmare removed the waiting-for-upstream We're waiting for a change upstream label Sep 2, 2020

MartinHjelmare merged commit 0892acb into home-assistant:dev Sep 2, 2020

thimic deleted the metno_api_v2_complete_endpoint branch September 2, 2020 20:32

leikoilja pushed a commit to leikoilja/core that referenced this pull request Sep 6, 2020

Met.no migrate from classic to complete endpoint (home-assistant#39493)

c34c066

springstan mentioned this pull request Sep 11, 2020

Use weather attribute conditions constants #39945

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Met.no migrate from classic to complete endpoint #39493

Met.no migrate from classic to complete endpoint #39493

thimic commented Aug 31, 2020 •

edited by MartinHjelmare

Loading

homeassistant commented Aug 31, 2020

probot-home-assistant bot commented Aug 31, 2020

Danielhiversen left a comment

frenck commented Aug 31, 2020

thimic commented Aug 31, 2020

MatthewFlamm commented Aug 31, 2020

MartinHjelmare left a comment •

edited

Loading

thimic commented Aug 31, 2020

MartinHjelmare commented Aug 31, 2020

thimic commented Aug 31, 2020 •

edited

Loading

thimic commented Sep 1, 2020

MartinHjelmare commented Sep 1, 2020

MartinHjelmare commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

MartinHjelmare commented Sep 2, 2020 •

edited

Loading

MartinHjelmare commented Sep 2, 2020

thimic commented Sep 2, 2020

MartinHjelmare left a comment

MartinHjelmare commented Sep 2, 2020

Met.no migrate from classic to complete endpoint #39493

Met.no migrate from classic to complete endpoint #39493

Conversation

thimic commented Aug 31, 2020 • edited by MartinHjelmare Loading

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

homeassistant commented Aug 31, 2020

probot-home-assistant bot commented Aug 31, 2020

Danielhiversen left a comment

Choose a reason for hiding this comment

frenck commented Aug 31, 2020

thimic commented Aug 31, 2020

MatthewFlamm commented Aug 31, 2020

MartinHjelmare left a comment • edited Loading

Choose a reason for hiding this comment

thimic commented Aug 31, 2020

MartinHjelmare commented Aug 31, 2020

thimic commented Aug 31, 2020 • edited Loading

thimic commented Sep 1, 2020

MartinHjelmare commented Sep 1, 2020

MartinHjelmare commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

thimic commented Sep 2, 2020

MartinHjelmare commented Sep 2, 2020 • edited Loading

MartinHjelmare commented Sep 2, 2020

thimic commented Sep 2, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 2, 2020

thimic commented Aug 31, 2020 •

edited by MartinHjelmare

Loading

Example entry for `configuration.yaml`:

MartinHjelmare left a comment •

edited

Loading

thimic commented Aug 31, 2020 •

edited

Loading

MartinHjelmare commented Sep 2, 2020 •

edited

Loading