Skip to content

Add missing AEMET weather units#70165

Merged
bdraco merged 2 commits intohome-assistant:devfrom
Noltari:aemet-weather-units
May 8, 2022
Merged

Add missing AEMET weather units#70165
bdraco merged 2 commits intohome-assistant:devfrom
Noltari:aemet-weather-units

Conversation

@Noltari
Copy link
Copy Markdown
Contributor

@Noltari Noltari commented Apr 16, 2022

Breaking change

AEMET weather pressure and wind speed were reported with the default units which didn't match the real units of those weather attributes (Pa vs hPa and m/s vs km/h).

Proposed change

Add missing AEMET weather units: #68619
Also add myself back to CODEOWNERS.

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:

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Copy link
Copy Markdown
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!

Should this be considered a breaking change since it affects data output?

@Noltari
Copy link
Copy Markdown
Contributor Author

Noltari commented Apr 16, 2022

Looks good!

Should this be considered a breaking change since it affects data output?

Changed to a Breaking change.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Apr 18, 2022
@Noltari
Copy link
Copy Markdown
Contributor Author

Noltari commented Apr 19, 2022

@frenck @Kane610 can you remove the bugfix label and add the breaking-change label?

@MatthewFlamm
Copy link
Copy Markdown
Contributor

This seems to be the same or very similar to #61471, which was not accepted.

@Noltari
Copy link
Copy Markdown
Contributor Author

Noltari commented Apr 20, 2022

This seems to be the same or very similar to #61471, which was not accepted.

As far as I know, #61471 became stale so it wasn't strictly rejected.
Also _attr_wind_speed_unit in that PR was wrong and it changed the hass.config.units.pressure_unit for the tests.
The default _attr_precipitation_unit matches the unit provided by AEMET (PRECIPITATION_MILLIMETERS_PER_HOUR).
In this case I'm only adding the units that differ with the original units reported by AEMET OpenData.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 27, 2022

While the code looks fine, I didn't merge because I want to understand all the history of this comment #61471 (comment) and how it affects this PR first

@Noltari
Copy link
Copy Markdown
Contributor Author

Noltari commented Apr 27, 2022

While the code looks fine, I didn't merge because I want to understand all the history of this comment #61471 (comment) and how it affects this PR first

Seems that those other mentioned integrations were changing the weather attributes units to the ones wanted for each integration "localization".
However, the AEMET integration is currently reporting values in hPa and km/h and this value is being assigned to the default units (Pa and m/s) without any conversion. Therefore, if AEMET is reporting for example a wind speed 15 km/h, right now this is displayed on the UI as 15 m/s, which is wrong and should be 4.17 m/s. And the same goes for pressure, if the value reported by AEMET is 1004.4 hPa, it is being displayed by the UI as 1004.4 Pa, which is wrong and should be 100440 Pa.

So, AFAIK, there are 2 issues here:

  1. Reporting weather attributes with the incorrect unit.
  2. Forcing the UI to display the weather attributes in the units typical of the integration "localization" (example: wind speed in km/h vs m/s vs knots).

@MatthewFlamm
Copy link
Copy Markdown
Contributor

There is some discussion here:

#60831 (comment)

@Noltari
Copy link
Copy Markdown
Contributor Author

Noltari commented Apr 27, 2022

There is some discussion here:

#60831 (comment)

Ok, so the problem was generated by #59533 in the first place...

@javierabion
Copy link
Copy Markdown

While the code looks fine, I didn't merge because I want to understand all the history of this comment #61471 (comment) and how it affects this PR first

Hello @bdraco : ¿Any news on this merge?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 6, 2022

Nope. Haven't had time to look into it

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 8, 2022

I spent a while reading the old PRs.

I think they are a bit of a red herring in this case as it looks like this is simply correcting the units to match how the api is reporting them.

@bdraco bdraco merged commit 9a5e028 into home-assistant:dev May 8, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2022
@Noltari Noltari deleted the aemet-weather-units branch May 10, 2022 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change bugfix cla-signed integration: aemet small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants