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

Zone Support #9

Closed
Mina-H-Samy opened this issue Jul 2, 2021 · 6 comments
Closed

Zone Support #9

Mina-H-Samy opened this issue Jul 2, 2021 · 6 comments

Comments

@Mina-H-Samy
Copy link

Hi,

Thanks for this it works great. I am close to getting zone support but need a tiny bit of help with a couple of things.
How do I push my changes to a branch in the repo for you to review?

Thanks

@Mina-H-Samy
Copy link
Author

Ok, I've gotten it to work. Changes are here.

It works but it is not very pretty. There are at least two things that need to be improved:

  1. switch.py uses an additional instance of MelView. I'm not sure how to have the same instance shared by both switch.py and climate.py.
  2. The configuration is duplicated atm. I am not sure how to share the login/pass between the two platforms.

@zacharyrs
Copy link
Owner

Hey,

Thanks for this and sorry for not getting back to you sooner - I've been swamped with work and uni.

I think you've proposed the PR against your own repo, so it won't merge into this one - that said I've got a few suggested changes.
I've made some of these suggestions on your PR, so after you've looked over them I'd be keen for you to add a PR to this repo.

I'd also like to tackle extra melview - I'm guessing we need a unified setup_platform function somehow... I'll read into that and see.

Cheers!

@Mina-H-Samy
Copy link
Author

No worries mate! I've addressed some of the feedback and commented on some other comments.
I've also raised a PR against this repo here: #12

Yeah that unified setup_platform would be great. I tried googling a bit but didn't find anything...

@Mina-H-Samy
Copy link
Author

Mina-H-Samy commented Jul 7, 2021

Maybe we don't need a unified setup_platform actually. I think we just need a class MelView that memoises the response.

def get_devices_list(self):
    if self.response is None:
        self.response = requests(...

    return self.response

@zacharyrs
Copy link
Owner

I did consider that, but I decided against it with the potential for having multiple MelView instances.

I'm actually not too sure on how it'll behave now, but it's something I do want to support.
I think unifying it is definitely the most suitable option - especially when looking at some of the other plugins for HA.
There'll probably be a larger rewrite with #11 to get back on top of that all.

@zacharyrs
Copy link
Owner

Closing as current progress has been merged into https://github.com/zacharyrs/ha-melview/tree/dev (#14).

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

No branches or pull requests

2 participants