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

Support for Point component #17466

Merged
merged 7 commits into from Nov 19, 2018
Merged

Support for Point component #17466

merged 7 commits into from Nov 19, 2018

Conversation

fredrike
Copy link
Contributor

@fredrike fredrike commented Oct 15, 2018

Description:

Adds support for the minut.com Point sensor/alarm.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7269

Example entry for configuration.yaml (if applicable): configuration.yaml

point:
    client_id: CLIENT_ID
    client_secret: CLIENT_SECRET  

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

Todo:

  • Smoother authentication (does HA have a preferred way to handle components with OAuth2?).
  • Integrate the new webhooks (where is the documentation/example code for this?)

homeassistant/components/sensor/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/binary_sensor/point.py Outdated Show resolved Hide resolved
homeassistant/components/binary_sensor/point.py Outdated Show resolved Hide resolved
@Kane610
Copy link
Member

Kane610 commented Oct 15, 2018

You should look at doing this integration through config entries.

homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/binary_sensor/point.py Outdated Show resolved Hide resolved
homeassistant/components/binary_sensor/point.py Outdated Show resolved Hide resolved
@fredrike
Copy link
Contributor Author

fredrike commented Oct 15, 2018

You should look at doing this integration through config entries.

@Kane610 Yes, I'm working on that.. I submitted this PR to get some suggestions on how to improve the integration.

I'll use this as inspiration of both the integration and the webhook part.

homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
@fredrike
Copy link
Contributor Author

We are required to support for:

  • Handles internet unavailable. Log a warning once when unavailable, log once when reconnected.
  • Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected.

for the Silver Quality Scale, what is the recommended way of doing so? Is this enough?

@Kane610
Copy link
Member

Kane610 commented Oct 16, 2018

For logging? That should be enough. Maybe some kind of identifier if you can connect to multiple clients

Don't forget that you have to move your MinutPointClient to an external lib

homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
homeassistant/components/point.py Outdated Show resolved Hide resolved
@fredrike
Copy link
Contributor Author

For logging? That should be enough. Maybe some kind of identifier if you can connect to multiple clients

Great!

What about SCAN_INTERVAL is there some example code I can look at on how it should be implemented or is this ok?
Or is it better to use track_time_interval(hass, self._sync, scan_interval)?

Don't forget that you have to move your MinutPointClient to an external lib

No I'm fully aware of that but have had so many changes that it's easier if everything is in the same place.

@Kane610
Copy link
Member

Kane610 commented Oct 16, 2018

@fredrike
Copy link
Contributor Author

scan interval is used like this:

home-assistant/homeassistant/components/switch/unifi.py

Line 24 in 6b3e4ca

SCAN_INTERVAL = timedelta(seconds=15)

And how is that used (I can't find a documentation for this)? Only the platform needs to update as that updates all sensors..

@Kane610
Copy link
Member

Kane610 commented Oct 16, 2018

Sometimes you need to ask the code ;)

Somewhere in entity platform or something will use that parameter to set the update interval for that platform.

@Kane610
Copy link
Member

Kane610 commented Oct 16, 2018

@fredrike fredrike requested a review from a team as a code owner October 19, 2018 07:01
homeassistant/components/point/point_api.py Outdated Show resolved Hide resolved
homeassistant/components/point/point_api.py Outdated Show resolved Hide resolved
homeassistant/components/point/point_api.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Show resolved Hide resolved
homeassistant/components/point/config_flow.py Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
@fredrike
Copy link
Contributor Author

@Kane610, I have a working configuration flow with authentication against minut.com now! Just a few questions:

screenshot 2018-10-19 at 9 30 42

screenshot 2018-10-19 at 9 46 10

@Kane610
Copy link
Member

Kane610 commented Oct 24, 2018

homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Show resolved Hide resolved
homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
@fredrike
Copy link
Contributor Author

@MartinHjelmare is my changes ok or are there more things that I need to fix?

What is the timeframe for 0.82.1?

@MartinHjelmare
Copy link
Member

I think this looks good, but I'd like to read the tests before I approve.

This will not go out before 0.83 since it's a new feature.

tests/components/point/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/point/test_config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/point/config_flow.py Outdated Show resolved Hide resolved
tests/components/point/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/point/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/point/test_config_flow.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare 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! @balloob?

@balloob
Copy link
Member

balloob commented Nov 19, 2018

I trust Martin :)

@balloob balloob dismissed their stale review November 19, 2018 11:52

Martin says it's ok

@balloob balloob merged commit c1ca7be into home-assistant:dev Nov 19, 2018
@ghost ghost removed the in progress label Nov 19, 2018
@balloob
Copy link
Member

balloob commented Nov 19, 2018

Beta hasn't been cut so it's part of 82.

@MartinHjelmare
Copy link
Member

I think that was a typo: 0.82 -> 0.83

@fredrike
Copy link
Contributor Author

@MartinHjelmare & @balloob, Thank you for your comments and improvements for enabling this. I really evaluate the code quality that you are striving for.

@balloob balloob mentioned this pull request Nov 29, 2018
@fredrike fredrike mentioned this pull request Dec 8, 2018
10 tasks
@fredrike fredrike mentioned this pull request Apr 22, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants