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 coordinator to QNAP #94413
Add coordinator to QNAP #94413
Conversation
@dgomes would you mind formatting me here? I’m just going to power through this quickly. |
Working with the web interface alone without testing it properly will not get this PR through :( |
Like @dgomes said, you should really setup a dev environment... it's not complicated and it will make it easier no only for the reviewers but also for yourself. |
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
I already began working on a brand new dev environment for this, I now see it’s necessary. |
Ok I think this one is ready |
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last small thing, regarding type hints.
Can you please also explicitely confirm that the code has been tested against a real QNAP server?
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Tested on QNAP server, working perfectly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@dgomes / @starkillerOG anything to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
One small comment related to the config flow PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS-251+
Breaking change
Proposed change
Moving QNAP integration to coordinator in prep for config_flow
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: