Add reolink IP NVR/Camera integration #84081
Conversation
home-assistant
bot
added
cla-signed
has-tests
integration: reolink
new-integration
labels
|
@JimStar would you also like to become codeowner of this (official) integration, since you are now also working actively on the custom component? @fwestenberg would you also like to become codeowner of this (official) integration, since you have made the original custom component? |
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
|
@bdraco thank you very much for the review! |
Not necessary, but thanks. At this moment I only use Frigate + addon. |
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
|
@bdraco I processed all feedback of the second review, could you review again/merge? |
bdraco
added
the
smash
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
|
@bdraco thanks for the last suggestion and approval. Does it need a second review, or can you merge this when the tests complete? |
👍
I always like to get a second set of 👀, but I don't see anything that I'm particularly concerned about here so should be good to merge if the CI passes. Corrections and improvements can always happen later. This looks like a good MVP |
|
|
||
| return True | ||
|
|
||
| async def update_states(self) -> bool: |
There was a problem hiding this comment.
Why does this return a boolean? What does that mean?
There was a problem hiding this comment.
I looked into the library. It seems the boolean represents if there's an error. Shouldn't we react to an error?
Looks like the library is both logging errors (at error level) and raising errors in some cases. That's usually bad practice. If the library has decided to raise an error, it's better to let the library user decide what to do with that error.
Debug logging is of course always ok.
There was a problem hiding this comment.
I would recommend to change the library to follow one practice only. Either trap all exceptions and only log errors, or don't log errors at all and just raise library specific exceptions. It's often easier to control the flow by using exceptions than checking booleans.
|
|
||
| async def update_states(self) -> bool: | ||
| """Call the API of the camera device to update the states.""" | ||
| return await self._api.get_states() |
There was a problem hiding this comment.
Side note: get_states sounds like it should return or retrieve more than one state. The naming and return value typing seems inconsistent.
Breaking change
Proposed change
Add Reolink NVR/camera integration.
Based on the custom components:
https://github.com/JimStar/reolink_cctv
https://github.com/fwestenberg/reolink_dev
Many thanks to @JimStar @fwestenberg for their work.
This is a stripped down version of the custom components that (schould) adhere to the HomeAssistant standards, from one I believe is that only a single platform can be added at a time.
Now only the camera platform is implemented.
Follow up PRs will implement binary_sensors (motion, people, vehicle. pet, doorbell detection), switches etc.
But first this basis needs to be accepted.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: