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

Add UniFi Uptime sensor #40058

Merged
merged 38 commits into from Sep 18, 2020
Merged

Add UniFi Uptime sensor #40058

merged 38 commits into from Sep 18, 2020

Conversation

timkoers
Copy link
Contributor

@timkoers timkoers commented Sep 14, 2020

Added the UniFi uptime data as a sensor. Untested.

Proposed change

New sensor feature, the UniFi Uptime 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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

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

Added the UniFi uptime data as a sensor. Untested.
@probot-home-assistant
Copy link

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

Updated code as a result of the tests.
@Kane610
Copy link
Member

Kane610 commented Sep 14, 2020

You should put this in draft state until it is ready for review

@MartinHjelmare MartinHjelmare changed the title Added UniFi Uptime sensor Add UniFi Uptime sensor Sep 14, 2020
Dev automation moved this from Needs review to Review in progress Sep 14, 2020
homeassistant/components/unifi/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/unifi/sensor.py Outdated Show resolved Hide resolved
@timkoers timkoers marked this pull request as draft September 14, 2020 09:27
Converted state to iso timestamp and changed device class to DEVICE_CLASS_TIMESTAMP.
timkoers and others added 2 commits September 14, 2020 11:56
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@Kane610
Copy link
Member

Kane610 commented Sep 14, 2020

Other things expected prior to approval:
Add option in options flow to enable/disable this
Add option property to controller class with default being disabled
Add tests

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Sep 14, 2020

If it's a single sensor the enabling/disabling can be done via the standard entity options already.

@Kane610
Copy link
Member

Kane610 commented Sep 14, 2020

It's per client so potentially a lot of entities

timkoers and others added 4 commits September 15, 2020 09:11
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
Removed uptime from the devices
@timkoers
Copy link
Contributor Author

timkoers commented Sep 15, 2020

@MartinHjelmare and @Kane610 I think everything should be fixed now!

@Kane610
Copy link
Member

Kane610 commented Sep 15, 2020

Nearly there! Smaller things left. Please also add a test to comply with the coverage comment; verify that the same client gets added twice

@timkoers
Copy link
Contributor Author

Nearly there! Smaller things left. Please also add a test to comply with the coverage comment; verify that the same client gets added twice

I've added the test that covers the uncovered line, including homeassistant/components/unifi/sensor.py#L58.

Copy link
Contributor Author

@timkoers timkoers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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!

Code owner should approve before merge.

@MartinHjelmare
Copy link
Member

Docs should be updated:
https://www.home-assistant.io/integrations/unifi/#sensor

timkoers added a commit to timkoers/home-assistant.io that referenced this pull request Sep 17, 2020
Copy link
Contributor Author

@timkoers timkoers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, all of them have been fixed

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

Awesome! Good job

Dev automation moved this from Review in progress to Reviewer approved Sep 18, 2020
@Kane610 Kane610 merged commit 46b86f4 into home-assistant:dev Sep 18, 2020
Dev automation moved this from Reviewer approved to Done Sep 18, 2020
@timkoers timkoers deleted the patch-1 branch September 20, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants