nkgilley / python-ecobee-api

Python API for controlling Ecobee Thermostats
MIT License
44 stars 40 forks source link

Add Set Vacation Mode functionality #43

Closed FuzzyMistborn closed 4 years ago

FuzzyMistborn commented 4 years ago

API: https://www.ecobee.com/home/developer/api/documentation/v1/functions/CreateVacation.shtml

Fixes https://github.com/nkgilley/python-ecobee-api/issues/42

(I'm still a bit of a python newb, but I tried to base this off existing functionality and just editing to fit the required calls)

marthoc commented 4 years ago

Can you also create a function to delete a vacation? See: https://www.ecobee.com/home/developer/api/documentation/v1/functions/DeleteVacation.shtml. That way I can add this functionality to Home Assistant as two companion services - one to create vacations and one to delete them.

marthoc commented 4 years ago

Nevermind - I see that the delete functionality already exists in the library.

My only suggestion is to rename the function to "create_vacation" as that is the name given to it by the ecobee API.

FuzzyMistborn commented 4 years ago

Yeah, the delete functionality has been there for a while it looks like. Made the changes to "create_vacation."

Would love this to be in HomeAssistant so I can automate vacations based on my Google Calendar but the coding required for that is well beyond me.

marthoc commented 4 years ago

Should be fairly trivial to add once this is added to the base library. Once @nkgilley is happy with this, merges it, and cuts a new release, I'll open a PR to add services in Home Assistant.

marthoc commented 4 years ago

I just reviewed more closely and it looks like you are missing a few other optional args: fan (The fan mode during the vacation) and fanMinOnTime (The minimum number of minutes to run the fan each hour). Make those kwargs at the end of the current call signature (maybe fan_mode and fan_min_on_time) and default them to "auto" and 0 (the defaults listed in the ecobee API).

My 2c - also I think that vacation_name should not be a keyword arg but a positional arg (it should be mandatory and not optional) since the user will need to know that name in order to delete the vacation I don't think the library should be providing a default - in which case move it after index but before cool_temp.

FuzzyMistborn commented 4 years ago

Yeah, I think I omitted those 2 because I would never use them/they would default to what I normally want which is fine. But I guess because it's in the API we should include it.

I was back and forth on whether to have a default for vacation name. From my testing it really doesn't seem to matter as the way the "delete_vacation" code is written it seems to delete it no matter what. I haven't been able to test it all that well. However, I think because the API states that it's a mandatory field it probably is a good idea to not have default language so I modified as you suggested.

nkgilley commented 4 years ago

Thanks!