-
-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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 Dremel 3D Printer integration #85969
Conversation
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.
General this looks good to me, but I cannot test it. Have only a question about the 2nd import.
…mel_3d_printer
api = await self.hass.async_add_executor_job(Dremel3DPrinter, host) | ||
except (ConnectTimeout, HTTPError, JSONDecodeError): | ||
errors = {"base": "cannot_connect"} | ||
except Exception: # pylint: disable=broad-except |
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.
Do you really need this broad exception? Is there no way to box this into known exception types?
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.
All the known exceptions are classified as cannot_connect
. Is the broad except not standard for config_flow
anymore?
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.
It’s alright, it gives the best user experience because the flow doesn’t crash, just says something bad happened. However, you should log the exception _LOGGER.exception(…)
to make sure we can see where the unknown error happened.
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.
Few changes necessary. Looking already really good.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Add the integration for the Dremel 3D Printer, models 3D20, 3D40 and 3D45. Dremel manufactures a high-tech 3D printer that does not integrate with OctoPrint. It has its own API built-in and, for the latest model (3D45), its own camera. This integration adds a config flow to add a Dremel 3D Printer based on its Host IP.
#72235 appears to have been abandoned. This is my own submission with much needed changes to keep up with the new standards. Only the sensor platform is exposed to start with to make review easier.
There is a custom integration in HACS so we already have it in the brands repo.
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: