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

Add method to update state from a callback #67

Merged
merged 2 commits into from
Dec 16, 2020

Conversation

steinerl
Copy link
Contributor

Hi,

I've added a method to update the JSON state of a NukiDevice from an external caller (e.g. a callback/webhook).

For context: I'm working on switching the Home Assistant Nuki component from polling to Bridge callbacks.

@pschmitt
Copy link
Owner

I don't get why this is required for what you are trying to do here. Is there some code I can look at?

@steinerl
Copy link
Contributor Author

The Nuki Bridge is going to call a Home Assistant webhook and the webhook handler should be able to update the state of the NukiDevice without directly accessing the _json attribute.

Have I overlooked something, is there already a clean method/setter?

@pschmitt
Copy link
Owner

Yeah, okay that makes sense. Setting _json directly is what I had in mind. But you are right, this isn't exactly the correct way.
I'd have implemented it differently though (using a property or adding an arg to the update function maybe?). Hmm, anyway imma merge this as is. Thanks!

@pschmitt pschmitt merged commit 33e354d into pschmitt:master Dec 16, 2020
@pschmitt
Copy link
Owner

This is part of 1.4.1

@steinerl
Copy link
Contributor Author

I thought about implementing it using a property too, but the _json object consists of 'device data' and 'state data' and the callback only updates 'state data', so a simple setter would be misleading IMHO.
I didn't want to touch/change the signature of the update method (for compatibility reasons) either.

Anyway, thanks for merging!

@steinerl steinerl deleted the feature/update_from_callback branch December 17, 2020 04:53
@Alfiegerner

This comment has been minimized.

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.

3 participants