dlarrick / hass-kumo

Home Assistant module interfacing with Mitsubishi mini-split units
MIT License
96 stars 21 forks source link

ip assign feature with requested changes #59

Closed omriasta closed 2 years ago

omriasta commented 2 years ago

This should include all the changes that @danielgoepp made to formatting as well as the requested changes to include "grandchildren". The last unresolved comment is still "...and here, and in the next loop. It may be worth a pair of utility functions to get / set the IPs for all the units" . I am not sure what you meant by this?

danielgoepp commented 2 years ago

Reran black and isort.

omriasta commented 2 years ago

I think this will work fine. Ideally the code to parse & edit the raw JSON would move to pykumo, but that can be done later so this gets into people's hands who need it. I'll merge this and push a beta version once I've done some local testing.

That sounds like a challenge...not sure how you would like pykumo to request the input from the user and then store it? Would there be a separate file just for IP addresses?

dlarrick commented 2 years ago

That sounds like a challenge...not sure how you would like pykumo to request the input from the user and then store it? Would there be a separate file just for IP addresses?

KumoCloudAccount has the raw JSON, held in memory in a member variable. My thought is to provide two new methods on KumoCloudAccount: get_raw_unit_info and set_unit_info or similar, to retrieve a list of dicts of info about the units, and allow editing this info into KumoCloudAccount's copy of the raw JSON. Initially it would only allow editing the IP address, but should the need arise later it could edit other things (like the unit name, as a dumb example). These functions would be used in hass-kumo in the 4 spots where the new code deals directly with the raw JSON (would still need to save the edited kumo_cache.json but that's fine).

Anyway, no need to do this now, but I anticipate getting called out in HA code review for this: it's something the pykumo library really should be responsible for.

omriasta commented 2 years ago

That sounds like a challenge...not sure how you would like pykumo to request the input from the user and then store it? Would there be a separate file just for IP addresses?

KumoCloudAccount has the raw JSON, held in memory in a member variable. My thought is to provide two new methods on KumoCloudAccount: get_raw_unit_info and set_unit_info or similar, to retrieve a list of dicts of info about the units, and allow editing this info into KumoCloudAccount's copy of the raw JSON. Initially it would only allow editing the IP address, but should the need arise later it could edit other things (like the unit name, as a dumb example). These functions would be used in hass-kumo in the 4 spots where the new code deals directly with the raw JSON (would still need to save the edited kumo_cache.json but that's fine).

Anyway, no need to do this now, but I anticipate getting called out in HA code review for this: it's something the pykumo library really should be responsible for.

Aha, that makes sense. I'll see if I can put something together when I get back from vacation... Doesn't sound too complicated...

danielgoepp commented 2 years ago

I'm not sure what happened with this pull request, but when I look at the commits, I do not see the merge indicating the branch relationship correctly. I see it as just hanging off master still:

Screen Shot 2021-09-11 at 4 12 33 AM

However if I do a merge locally, it fixes it and shows the merge properly:

Screen Shot 2021-09-11 at 4 17 47 AM

There is no actual change to master, it just establishes the relationship with the branch to join their histories. I did not push this, but I could. I didn't want to do anything to master without checking with @dlarrick first. Thoughts?

dlarrick commented 2 years ago

Ah, I did squash-and-merge to keep master's revision history clean. We should delete the feature branch.

danielgoepp commented 2 years ago

Sound good, done. Thanks.