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

Backwards compatibility handling on last_event for button and relative_rotary #394

Conversation

idekker
Copy link
Contributor

@idekker idekker commented Aug 24, 2024

Added code to handle the case where the bridge only emits the deprecated last_event property and not button_report (or relative_rotary). Also created a test for it.

I do wonder whether this code has to move to the ButtonController (and RelativeRotaryController) class before _handle_event on the base class is called, including the code that drops update-events that do not have a report. But I'm not sure if any of the code that is after super()._handle_event(...) is still relevant even if we decided not to inject the event/new resource upstream.

@idekker idekker force-pushed the last_event_backwards_compatibility branch from f7a95ad to 30d925f Compare August 24, 2024 09:39
@idekker idekker marked this pull request as draft August 24, 2024 14:16
@marcelveldt
Copy link
Collaborator

I like the fact that you added some tests but I';m not sure if we really need to be deprecated reports. In my experience, users are either running the EOL v1 bridge or a up-to-date V2 bridge. This change with the button events is already in the Hue firmware since about a year so I think we're passed the station where we actually still need to be backward compatible.

@idekker
Copy link
Contributor Author

idekker commented Oct 1, 2024

I agree. If they are not running recent firmware they'll also miss other features and support for newer devices.
I can remove the new functionality and just make a PR with some tests.

@marcelveldt
Copy link
Collaborator

Thanks!

@idekker
Copy link
Contributor Author

idekker commented Oct 5, 2024

As discussed, closing this one and created #419 with just the tests.

@idekker idekker closed this Oct 5, 2024
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.

2 participants