-
-
Notifications
You must be signed in to change notification settings - Fork 31.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
Envisalink support #2304
Envisalink support #2304
Conversation
Merge remote-tracking branch 'upstream/dev' into evl3
|
||
config = base_config.get(DOMAIN) | ||
|
||
if config.get(CONF_EVL_HOST) is None: |
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.
Instead of doing all these error checks, you could consider using a voluptuous schema like for example MQTT uses. Naming it CONFIG_SCHEMA
will trigger Home Assistant to validate your config before it arrives in setup(hass, config)
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.
Oh, and using vol.Coerce(int)
you can also force data to be an integer etc.
Wow good job 👍 A few minor comments but this looks very good! ⭐⭐⭐⭐⭐ |
Awesome- thanks! It's been a relatively long time in the making... Also, I'll get those commits squashed/documentation submitted- it was late, and I had to merge the dev branch in a few times during development, so the squash was not exactly trivial. I wanted to get it in front of you at least. |
Looks like you violated some style guide lines. From your CI build: https://travis-ci.org/home-assistant/home-assistant/jobs/137997755#L197-L278 |
Oh- sorry wasn't quite ready for you to validate again- I'll get these fixed ASAP. |
Alright, I'm all set! However I'm struggling quite a bit with getting everything squashed with the pulls I did in the middle- do you have any tricks for dealing with that? |
vol.Required(CONF_USERNAME): cv.string, | ||
vol.Required(CONF_PASS): cv.string, | ||
vol.Required(CONF_CODE): cv.string, | ||
vol.Optional(CONF_ZONES): dict, |
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.
If this is going to be a dictionary mapping integers (zone_num) to a schema, you can use this:
vol.Optional(CONF_ZONES): {vol.Coerce(int): ZONE_SCHEMA},
This will validate all keys as integers and each value against ZONE_SCHEMA.
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- yes i was definitely trying to figure that out. When I couldn't- that's when I put further validation within the sensor/binary_sensor/alarm_panel. I'll take that logic back out since this is now working. It will make those codebases smaller/simpler.
Thanks for your help- these changes have been made. In terms of the squash- I'm actually wondering if pulling these commits into a new branch and re-submitting might be easiest/cleanest... |
Don't worry about the squashing, GitHub has a built in option now to do it when I press merge. |
Excellent- good to know. Thanks! |
Description:
This is a new platform, with several components to control and show the status of any alarm panel driven by the Eyez-on envisalink platform. This includes both DSC and Honeywell panels.
Related issue (if applicable): Enhancement request 409
https://community.home-assistant.io/t/dsc-alarm-integration/409
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If code communicates with devices:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass