Add binary_sensor to Starlink by boswelja · Pull Request #85409 · home-assistant/core

boswelja · 2023-01-08T01:25:37Z

Proposed change

Adding the binary_sensor platform to the Starlink integration. This primarily takes data from all "alerts" that the Starlink system exposes, which may change at any given time.

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)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Update Starlink documentation for new binary_sensor platform home-assistant.io#25637

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

To help with the load of incoming pull requests:

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

boswelja · 2023-01-08T01:38:02Z

Not sure what's going on with mypy, nothing is declared as Any in code and I don't see those errors locally 😕

bdraco · 2023-01-08T02:01:26Z

Are you missing a py.typed file in the upstream lib?
https://peps.python.org/pep-0561/

boswelja · 2023-01-08T02:06:51Z

Possibly, we did have issues with typing due to the wide range of supported Python versions. If that's the case, do we just need an empty marker file py.typed? Also, why would it not have been flagged before?

bdraco · 2023-01-08T02:16:14Z

Possibly, we did have issues with typing due to the wide range of supported Python versions. If that's the case, do we just need an empty marker file py.typed?

seems so

Also, why would it not have been flagged before?

homeassistant.components.starlink.* was added to .strict-typing in this PR

boswelja · 2023-01-08T02:34:22Z

Interesting that it didn't flag it locally with that change, but I will roll that back for now. Thanks!

bdraco · 2023-01-08T02:36:17Z

Interesting that it didn't flag it locally with that change, but I will roll that back for now. Thanks!

You probably wouldn't see it until mypy.ini gets rebuilt and mypy was run again

boswelja · 2023-01-09T07:45:10Z

I have switched to more standard binary_sensor declarations, and updated the documentation PR to reflect changes.

bdraco · 2023-01-09T08:00:55Z

Looks good. 👍 Some minor comments above.

I'll try to find a time this week where I can reset again and test

boswelja · 2023-01-09T10:30:39Z

I probably should've given you a heads-up to avoid re-resetting for this 😅
Next up I want to look at buttons, once this is merged I'll open up another PR

Co-authored-by: J. Nick Koston <nick@koston.org>

bdraco · 2023-01-12T03:09:58Z

+    StarlinkBinarySensorEntityDescription(
+        key="install_pending",
+        name="Update pending",
+        device_class=BinarySensorDeviceClass.UPDATE,


https://developers.home-assistant.io/docs/core/entity/binary-sensor?_highlight=binarys

On means update available, Off means up-to-date. The use of this device class should be avoided, please consider using the update entity instead.

This should be implemented as an update entity instead

I will look into the update platform next. Buttons (and switches) are relatively small, would it make sense to tack update on the end of the same PR?

Better to do one platform per PR. Its a lot easier to find small blocks of time to do reviews. Its the big PRs that sit for a long time because they need large chunks of time

On update platform - how should I handle this situation? Starlink doesn't expose the "latest version" as far as we know, it just says "there is an update"

We talked about this and it looks like that ends up falling into the exception case since there is no good solution currently when you don't know the new version.

So good to add back in a new PR. Sorry for the churn.

bdraco · 2023-01-12T03:12:25Z

Did the reset song and dance to get dishy out of bypass mode.

Looks good.

I'll take care of dropping the update binary sensor so you don't have to wait for me to retest again

bdraco · 2023-01-12T03:12:48Z

MartinHjelmare

Please address the comment in a new PR. Thanks!

MartinHjelmare · 2023-01-16T09:00:22Z

@@ -12,7 +15,11 @@
 COORDINATOR_SUCCESS_PATCHER = patch.object(
    StarlinkUpdateCoordinator,


We should patch the library and not the coordinator. The coordinator is a detail of the integration.

https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations

home-assistant Bot added cla-signed has-tests integration: starlink new-feature new-platform by-code-owner Quality Scale: No score labels Jan 8, 2023

boswelja mentioned this pull request Jan 8, 2023

Add Starlink Integration #77091

Merged

23 tasks

boswelja mentioned this pull request Jan 8, 2023

Update Starlink documentation for new binary_sensor platform home-assistant/home-assistant.io#25637

Merged

8 tasks

boswelja marked this pull request as ready for review January 8, 2023 06:36

boswelja force-pushed the feature/starlink-binary-sensors branch from 208cac7 to 11d725c Compare January 8, 2023 06:36

bdraco reviewed Jan 8, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

bdraco reviewed Jan 8, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

boswelja force-pushed the feature/starlink-binary-sensors branch from e76895e to 632279d Compare January 9, 2023 02:55

bdraco reviewed Jan 9, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/__init__.py Outdated

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

bdraco reviewed Jan 9, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

bdraco reviewed Jan 9, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

boswelja added 4 commits January 10, 2023 20:00

Update coordinator and entity to make sure we can access alert info

123c3c4

Initial implementation of binary sensors

9df79d5

Add "currently obstructed" binary sensor

027956c

Fix test patcher

63e7e40

boswelja and others added 13 commits January 10, 2023 20:00

Add starlink to strict-typing

f584ff9

Add binary_sensor to coveragerc

7bb6c39

Remove Starlink from strict-typing

16637df

Update some device_classes

7b412b4

Move domain-specific entities to the appropriate files

e985a6e

Hardcode binary sensors

0676df4

Fix inverted obstruction state

8aa18b3

Touch up binary sensors

009a759

Update homeassistant/components/starlink/binary_sensor.py

096734f

Co-authored-by: J. Nick Koston <nick@koston.org>

Sort platforms alphabetically

96f108e

Update binary_sensor.py

50ffb8a

Create StarlinkData data class

6dc6074

Fix up patchers

d272fc9

boswelja force-pushed the feature/starlink-binary-sensors branch from ea4a439 to d272fc9 Compare January 10, 2023 07:00

bdraco reviewed Jan 11, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/coordinator.py Outdated

Apply suggestion from code review

27f7a57

bdraco reviewed Jan 12, 2023

View reviewed changes

Comment thread homeassistant/components/starlink/binary_sensor.py Outdated

bdraco added 2 commits January 11, 2023 17:13

Update homeassistant/components/starlink/binary_sensor.py

8445dc0

Merge branch 'dev' into feature/starlink-binary-sensors

b7f03fe

bdraco approved these changes Jan 12, 2023

View reviewed changes

bdraco merged commit 43cc8a1 into home-assistant:dev Jan 12, 2023

boswelja deleted the feature/starlink-binary-sensors branch January 12, 2023 04:09

github-actions Bot locked and limited conversation to collaborators Jan 13, 2023

MartinHjelmare reviewed Jan 16, 2023

View reviewed changes

home-assistant unlocked this conversation Jan 16, 2023

github-actions Bot locked and limited conversation to collaborators Jan 17, 2023

		@@ -12,7 +15,11 @@
		COORDINATOR_SUCCESS_PATCHER = patch.object(
		StarlinkUpdateCoordinator,

Uh oh!

Conversation

boswelja commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

boswelja commented Jan 8, 2023

Uh oh!

bdraco commented Jan 8, 2023

Uh oh!

boswelja commented Jan 8, 2023

Uh oh!

bdraco commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boswelja commented Jan 8, 2023

Uh oh!

bdraco commented Jan 8, 2023

Uh oh!

Uh oh!

Uh oh!

boswelja commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdraco commented Jan 9, 2023

Uh oh!

boswelja commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

bdraco Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boswelja Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

bdraco Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

boswelja Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

bdraco Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdraco commented Jan 12, 2023

Uh oh!

bdraco commented Jan 12, 2023

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

boswelja commented Jan 8, 2023 •

edited

Loading

bdraco commented Jan 8, 2023 •

edited

Loading

bdraco Jan 12, 2023 •

edited

Loading

bdraco Jan 12, 2023 •

edited

Loading