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

Set last_service_data in the right place #652

Merged
merged 7 commits into from
Aug 1, 2023
Merged

Conversation

basnijholt
Copy link
Owner

I noticed this changed after #598

@protyposis, was this intentional?

@protyposis
Copy link
Collaborator

What exactly do you mean re #598? I can't see any last_service_data-related changes in there.

To be honest, I never completely understood its purpose, and so I tried to keep this as untouched as I could. But the change you made in here seems reasonable :)

@th3w1zard1
Copy link
Collaborator

iirc the last time we touched last_service_data was during the migration from the old detect_non_ha_changes code. This PR's change would affect #512 but it doesn't seem like we're planning on using that PR anyway.

@basnijholt
Copy link
Owner Author

basnijholt commented Jul 26, 2023

I believe the only thing that changed is that currently the code can set self.manager.last_service_data[light] = service_data and in some next step it can terminate early before light.turn_on has been called.

Upon further inspection I think this shouldn't matter and I don't think it breaks anything.

I have been reading all changes in detail since there have been some unexplainable errors that people have reported 😅

I also realized that it's always been incorrect for the separate_turn_on_commands because there the last_service_data is set to the pre-split version of it. This PR is the correct fix I believe.

@basnijholt basnijholt merged commit aa9f69a into main Aug 1, 2023
14 checks passed
@basnijholt basnijholt deleted the set-last-service-data branch August 1, 2023 00:52
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