auchter / haaska

Home Assistant Alexa Skill Adapter
263 stars 61 forks source link

Cleanup #39

Closed robbiet480 closed 7 years ago

robbiet480 commented 7 years ago
robbiet480 commented 7 years ago

@auchter Can this get looked at and hopefully merged soon? Thanks!

auchter commented 7 years ago

Yeah, apologies for it taking so long. I spent some time reviewing / merging it this past weekend (along with a change from @trisk). That work's currently living in the dev branch: https://github.com/auchter/haaska/tree/dev (note: I tend to rebase this branch, so probably not the best to base any work on...)

Anyway, overall I'm in favor of these changes. If you look at the dev branch, you'll notice that I did end up making some changes. I agree that the new keys for configuration are perhaps clearer, I don't like the idea of an upgrading haaska breaking a working configuration (particularly when the change is primarily cosmetic). So, I changed the code to accept the old keys, while updating the documentation to the new, cleaner key names.

I also added a quick hack to convert an existing configuration file to one with the new key names by running make modernize_config.

Let me know if you've got any comments/feedback/flames on the changes I've made in the dev branch. I've got a few more things outstanding that I want to finish up before merging to master, but I'll aim to get this stuff merged by Sunday.

Thanks again for the contribution!

robbiet480 commented 7 years ago

@auchter dev looks good. We should ensure that the documentation and default allowed_domains values are synced, it appears climate and fan are missing from the docs but exist in allowed_domains. I also think that the ssl_verify parameter is a little misleading, as I would understand it to be a boolean which enables/disables SSL verification but it appears it actually expects a path to a certificate to very against?

trisk commented 7 years ago

@robbiet480 it's not obvious, but the parameter also works as a boolean! true makes it use the built-in CA bundle.