Skip to content

Add agent_dvr integration#32711

Merged
bdraco merged 25 commits intohome-assistant:devfrom
ispysoftware:agent
May 8, 2020
Merged

Add agent_dvr integration#32711
bdraco merged 25 commits intohome-assistant:devfrom
ispysoftware:agent

Conversation

@ispysoftware
Copy link
Copy Markdown
Contributor

@ispysoftware ispysoftware commented Mar 12, 2020

Proposed change

This adds an integration for Agent DVR, a windows 10 based DVR solution - https://www.ispyconnect.com/download.aspx

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @ispysoftware,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @ispysoftware,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

1 similar comment
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @ispysoftware,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 12, 2020

This is a really large PR.

A new integration should be limited to a single platform. As per:

https://developers.home-assistant.io/docs/creating_component_code_review#5-make-your-pull-request-as-small-as-possible

This makes reviewing easier to do, and in the end, everybody will be able to make progress quicker.

@ispysoftware
Copy link
Copy Markdown
Contributor Author

@frenck I removed the code for binary sensors and the alarm control panel.

Comment thread .coveragerc Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 13, 2020

OK, in general, I think it looks good. However, a second reviewer has to take a look as well.

Last comment from me, would be the quality scale index you are trying to add: Gold (according to your initial openings post). In the current state of the integration, this level isn't reached.

See: https://developers.home-assistant.io/docs/integration_quality_scale_index/.

For example, tests for fetching data from the integration and controlling it are missing. Please make sure the integration matches the level selected. Another option would be to lower the quality scale and improve on it in future PR's.

@ispysoftware
Copy link
Copy Markdown
Contributor Author

ispysoftware commented Mar 14, 2020

OK no probs, i changed it to silver. Thanks for reviewing it and being so responsive. It's much appreciated.

@ispysoftware
Copy link
Copy Markdown
Contributor Author

Hey @frenck how do i get someone else to review it? I've made a load more updates to it but don't want to commit them until this is approved as then I guess you'd need to review it again...

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 17, 2020

By waiting for it. We have quite the workload, so it might take a moment.

@ispysoftware
Copy link
Copy Markdown
Contributor Author

Hey @frenck, any update or estimated time frame? Does this usually take weeks/ months?

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 31, 2020

It is an opensource project, we rely on people investing their spare free time to contribute to this project. Sometimes this happens quickly, sometimes it takes a bit. Please, have some patience, thanks.

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Greetings @ispysoftware Thanks for submitting this PR. There are a few items that need adjustment.

Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/config_flow.py Outdated
Comment thread homeassistant/components/agent_dvr/config_flow.py Outdated
Comment thread homeassistant/components/agent_dvr/config_flow.py Outdated
Comment thread homeassistant/components/agent_dvr/config_flow.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/config_flow.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/helpers.py Outdated
Comment thread homeassistant/components/agent_dvr/services.yaml Outdated
Comment thread homeassistant/generated/zeroconf.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

You might save yourself a lot of headache by installing the pre commit hooks that will do all the formatting for you. https://developers.home-assistant.io/docs/development_environment/#setting-up-virtual-environment

@ispysoftware
Copy link
Copy Markdown
Contributor Author

I had it all setup with tox and black and everything working locally but had to revert 6 weeks worth of work to do these changes and now docker isn't working properly, my git repo is a mess, our zeroconf code is broken and im currently trying to bring all our test linux computers back to a point where all this old stuff was relevant.

Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/const.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label May 6, 2020
@ispysoftware
Copy link
Copy Markdown
Contributor Author

So what is this waiting on now?

Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/__init__.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/camera.py Outdated
ispysoftware and others added 7 commits May 6, 2020 20:42
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Copy link
Copy Markdown
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! Just a couple of clean ups left.

Comment thread homeassistant/components/agent_dvr/camera.py Outdated
Comment thread homeassistant/components/agent_dvr/manifest.json Outdated
Copy link
Copy Markdown
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.

Great!

@MartinHjelmare
Copy link
Copy Markdown
Member

@bdraco ok?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 8, 2020

@bdraco ok?

Will run though again on my next break from meetings

Copy link
Copy Markdown
Member

@bdraco bdraco 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. Thanks for jumping in @MartinHjelmare 🥇

@bdraco bdraco merged commit 1be41b9 into home-assistant:dev May 8, 2020
@lock lock Bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants