Skip to content
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

ip assign feature with requested changes #59

Merged
merged 4 commits into from
Sep 9, 2021
Merged

Conversation

omriasta
Copy link
Collaborator

@omriasta omriasta commented Sep 8, 2021

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
Copy link
Collaborator

Reran black and isort.

Copy link
Owner

@dlarrick dlarrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@omriasta
Copy link
Collaborator Author

omriasta commented Sep 8, 2021

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
Copy link
Owner

dlarrick commented Sep 9, 2021

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.

@dlarrick dlarrick merged commit e873e97 into master Sep 9, 2021
@omriasta
Copy link
Collaborator Author

omriasta commented Sep 9, 2021

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
Copy link
Collaborator

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
Copy link
Owner

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

@danielgoepp danielgoepp deleted the ip-assign-feature branch September 11, 2021 11:32
@danielgoepp
Copy link
Collaborator

Sound good, done. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants