-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Upgrade HomeWizard to platinum quality #82580
Conversation
As you're going from nothing to platinum I think it would be good to document what (and what Not) you're fulfilling on the quality scale |
@Kane610 thanks for the suggestion, will work on that 👍 |
Main description has been updated with the list. The line "From nothing to platinum".. is it more common that we take smaller steps? We can do that as well of course. |
There is no expected "going the line" procedure, but you should fulfil the different steps, so maybe expand what is needed for silver and put up a PR for that, etc. |
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.
For the silver level, this wasn't checked from the list:
Operations like service calls and entity methods (e.g. Set HVAC Mode) have proper exception handling. Raise ValueError on invalid user input and raise HomeAssistantError for other failures such as a problem communicating with a device. -> NA
Although it was set to be NA, that isn't correct. It does apply. Some example:
- The button entity (identify) could throw a connection error / could fail. However, this is not handled (and should be throwing an
HomeAssistantError
. - The number entity, setting a value could fail in a similar way (and should also be throwing an
HomeAssistantError
in such cases, unless the value is unexpected, in that case aValueError
should be raised. Currently, these services have no error handling at all. - Turning a switch entity on/off imposes similar issues as the above two mentioned ones.
Good catch, not considered. Should I change this in this PR or a separate to keep this one clean? |
I would drop those in separate PR, and maybe mark this PR draft until it has been resolved. |
Done! Let me know if you see any other potential issue |
Yeah, thanks for the reminder! |
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.
Awesome! Thank you! |
Proposed change
I think this integration checks all (or at least most) the boxes to mark it as platinum.
Quality index
Below the list of requirements, checked if I think the requirement is met (and/or with comments)
No score
Silver 🥈
Gold 🥇
Platinum 🏆
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: