Add brightsky weather entity #37800

mweinelt · 2020-07-12T22:58:21Z

Breaking change

Proposed change

Add support for DWD weather data using the Bright Sky API.

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

Link to documentation pull request: Add documentation for the Bright Sky weather entity home-assistant.io#13987

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
Add documentation for the Bright Sky weather entity home-assistant.io#13987

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

TODOs

Tests are still on my TODO list
The weather condition is not currently exposed by bright sky, I've opened an issue upstream and it's on their todo list. Weather condition jdemaeyer/brightsky#85

Notes

It's violating the rule about requests to external services being outsourced into dedicated Python Packages. That makes little sense to me for ~60 LOC that do two HTTP GET requests and very little else.

frenck · 2020-07-13T18:03:17Z

We no longer accept integrations, that connect with a device or service, to be configured via YAML.
See: https://github.com/home-assistant/architecture/blob/master/adr/0010-integration-configuration.md#decision

Please add a configuration flow to this integration, see our developers documentation for more information: https://developers.home-assistant.io/docs/config_entries_config_flow_handler

Thanks already 👍

frenck

Adding review comment to correct the state of this PR.

mweinelt · 2020-08-03T21:31:02Z

I have this in a somewhat working state and have been using it for a couple of weeks. I have no clue if what I've done with the config flow and its test is sensible.

The API currently has the limitation of only relying on data collected from stations, filled up with data from other stations, when the closest station does not offer a certain feature. This means that data like the weather condition could come from a station as much as 30km away.

Another limitation is that there is no interpolation of data in relation to the distance one has to the next station. So the data has to be taken with a grain of salt.

My next station is 3 km away across the city and the values measured are spot on, so there is certainly some value.

RobBie1221 · 2020-09-02T19:12:16Z

It's violating the rule about requests to external services being outsourced into dedicated Python Packages. That makes little sense to me for ~60 LOC that do two HTTP GET requests and very little else.

The developer docs mention that all API specific code should be part of a third party library. It's not just about 2 HTTP GET requests, you are mapping brightsky keys to ha keys here, which is API specific code. That should be put in a separate library such that when API's change, ha only needs a dependency bump and the logic can be changed in the third party library.

disrupted · 2020-09-09T07:34:01Z

There's an official library on PyPI for brightsky now: https://pypi.org/project/brightsky/

mweinelt · 2020-09-09T14:22:55Z

I wouldn't recommend running your own brightsky instance, when there is an API that can be easily queried.

@RobBie1221 Okay, rescheduling work on this for some time in october, can't get to it earlier than that.

RobBie1221 · 2020-09-09T16:36:10Z

API specific code needs to abstracted to a package hosted on PyPi. @disrupted is pointing to an existing package which can be used.

disrupted · 2020-09-09T16:52:49Z

I might've gotten it wrong. Looks like the PyPI package is for running your own instance as @mweinelt noted. If we just want to query the official instance https://brightsky.dev/ I guess we should build a wrapper around that or ask the developer if he's planning to make this functionality available within the brightsky package.

stale · 2020-10-12T00:07:57Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

mweinelt · 2020-10-18T22:31:58Z

I'm working on separating API access and hass integration right now.

mweinelt · 2020-10-19T00:15:47Z

Published hass-brightsky-client at https://pypi.org/project/hass-brightsky-client/ and updated the component to make use of the library.

Untested as of yet.

mweinelt · 2020-12-19T23:41:32Z

Squashed and rebased.

mweinelt · 2020-12-19T23:42:37Z

API specific code needs to abstracted to a package hosted on PyPi. @disrupted is pointing to an existing package which can be used.

This was fixed.

MartinHjelmare

Please exclude non tested modules from coverage by including them in .coveragerc. Only the config flow requires 100 % coverage, although more tests are appreciated.

MartinHjelmare · 2020-12-20T00:48:55Z

homeassistant/components/brightsky/__init__.py

+CONFIG_SCHEMA = vol.Schema({DOMAIN: vol.Schema({})}, extra=vol.ALLOW_EXTRA)
+


Suggested change

CONFIG_SCHEMA = vol.Schema({DOMAIN: vol.Schema({})}, extra=vol.ALLOW_EXTRA)

MartinHjelmare · 2020-12-20T00:50:52Z

homeassistant/components/brightsky/config_flow.py

+            try:
+                info = await validate_input(self.hass, user_input)
+
+                return self.async_create_entry(title=info["title"], data=user_input)


Please only wrap the line than can raise in the try... except block. Use and else: block if needed.

MartinHjelmare · 2020-12-20T00:51:32Z

homeassistant/components/brightsky/config_flow.py

+                vol.Required(
+                    CONF_LONGITUDE, default=self.hass.config.longitude
+                ): cv.longitude,
+                vol.Optional(CONF_MODE, default="hourly"): vol.In(FORECAST_MODE),


If this option isn't required for the connection it should be moved to an options flow instead of the main flow.

MartinHjelmare · 2020-12-20T00:53:07Z

homeassistant/components/brightsky/manifest.json

@@ -0,0 +1,11 @@
+{
+  "domain": "brightsky",


We normally separate words with underscore.

Suggested change

"domain": "brightsky",

"domain": "bright_sky",

MartinHjelmare · 2020-12-20T00:54:35Z

homeassistant/components/brightsky/strings.json

+      "unknown": "[%key:common::config_flow::error::unknown%]"
+    },
+    "abort": {
+      "already_configured": "[%key:common::config_flow::abort::already_configured_device%]"


This string isn't used.

MartinHjelmare · 2020-12-20T00:56:13Z

homeassistant/components/brightsky/weather.py

+        return self._name
+
+    @property
+    def state(self) -> str:


Don't implement state. Implement the condition property.

MartinHjelmare · 2020-12-20T00:58:39Z

homeassistant/components/brightsky/weather.py

+        return items
+
+    @Throttle(UPDATE_INTERVAL)
+    def update(self) -> None:


Consider using our data update coordinator instead.

https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities

MartinHjelmare · 2020-12-20T00:59:32Z

tests/components/brightsky/test_config_flow.py

+        "homeassistant.components.brightsky.async_setup_entry",
+        return_value=True,
+    ) as mock_setup_entry:
+        result2 = await hass.config_entries.flow.async_configure(


We need to patch the library here to avoid I/O.

mweinelt · 2021-02-27T01:15:59Z

Sorry, I've lost interest in completing this.

homeassistant added the cla-signed label Jul 12, 2020

probot-home-assistant bot added new-integration integration: brightsky labels Jul 12, 2020

project-bot bot added this to Needs review in Dev Jul 12, 2020

mweinelt force-pushed the brightsky branch 3 times, most recently from 28d130c to f1bd2ae Compare July 12, 2020 23:17

mweinelt mentioned this pull request Jul 13, 2020

Add documentation for the Bright Sky weather entity home-assistant/home-assistant.io#13987

Closed

8 tasks

mweinelt force-pushed the brightsky branch from f1bd2ae to e1f7db1 Compare July 13, 2020 22:25

mweinelt force-pushed the brightsky branch 5 times, most recently from b26c97a to e211ffa Compare July 24, 2020 22:26

mweinelt mentioned this pull request Jul 25, 2020

Weather-forecast card shows precipitation for tomorrow below current temperature home-assistant/frontend#6461

Closed

3 tasks

mweinelt force-pushed the brightsky branch from e211ffa to 56f4007 Compare July 25, 2020 15:09

mweinelt mentioned this pull request Jul 25, 2020

met: forecast timestamps are not in UTC #38193

Closed

Dev automation moved this from Needs review to Review in progress Aug 3, 2020

frenck requested changes Aug 3, 2020

View reviewed changes

frenck added this to Incoming in New Integrations via automation Aug 3, 2020

frenck moved this from Incoming to Awaiting Requested Changes in New Integrations Aug 3, 2020

mweinelt force-pushed the brightsky branch from 56f4007 to 882a9b7 Compare August 3, 2020 21:23

mweinelt marked this pull request as ready for review August 3, 2020 21:24

mweinelt force-pushed the brightsky branch from 882a9b7 to c63b8c2 Compare August 25, 2020 17:23

stale bot added the stale label Oct 12, 2020

stale bot removed the stale label Oct 18, 2020

mweinelt force-pushed the brightsky branch from c63b8c2 to e4ca20c Compare October 19, 2020 00:14

mweinelt force-pushed the brightsky branch from e4ca20c to a4a2ddd Compare October 19, 2020 00:18

This comment has been minimized.

Sign in to view

Add brightsky component

7854ae3

mweinelt force-pushed the brightsky branch from 1a36713 to 7854ae3 Compare December 19, 2020 23:41

MartinHjelmare requested changes Dec 20, 2020

View reviewed changes

mweinelt closed this Feb 27, 2021

Dev automation moved this from Review in progress to Cancelled Feb 27, 2021

New Integrations automation moved this from Awaiting Requested Changes to Cancelled Feb 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add brightsky weather entity #37800

Add brightsky weather entity #37800

mweinelt commented Jul 12, 2020 •

edited

frenck commented Jul 13, 2020

frenck left a comment

mweinelt commented Aug 3, 2020

RobBie1221 commented Sep 2, 2020

disrupted commented Sep 9, 2020

mweinelt commented Sep 9, 2020 •

edited

RobBie1221 commented Sep 9, 2020

disrupted commented Sep 9, 2020

stale bot commented Oct 12, 2020

mweinelt commented Oct 18, 2020

mweinelt commented Oct 19, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

mweinelt commented Dec 19, 2020

mweinelt commented Dec 19, 2020

MartinHjelmare left a comment

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

MartinHjelmare Dec 20, 2020

mweinelt commented Feb 27, 2021

		CONFIG_SCHEMA = vol.Schema({DOMAIN: vol.Schema({})}, extra=vol.ALLOW_EXTRA)

Add brightsky weather entity #37800

Add brightsky weather entity #37800

Conversation

mweinelt commented Jul 12, 2020 • edited

Breaking change

Proposed change

Type of change

Additional information

Checklist

TODOs

Notes

frenck commented Jul 13, 2020

frenck left a comment

Choose a reason for hiding this comment

mweinelt commented Aug 3, 2020

RobBie1221 commented Sep 2, 2020

disrupted commented Sep 9, 2020

mweinelt commented Sep 9, 2020 • edited

RobBie1221 commented Sep 9, 2020

disrupted commented Sep 9, 2020

stale bot commented Oct 12, 2020

mweinelt commented Oct 18, 2020

mweinelt commented Oct 19, 2020 • edited

This comment has been minimized.

This comment has been minimized.

mweinelt commented Dec 19, 2020

mweinelt commented Dec 19, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweinelt commented Feb 27, 2021

mweinelt commented Jul 12, 2020 •

edited

mweinelt commented Sep 9, 2020 •

edited

mweinelt commented Oct 19, 2020 •

edited