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

Fix SSL Regression #16

Merged
merged 10 commits into from
Aug 11, 2024
Merged

Fix SSL Regression #16

merged 10 commits into from
Aug 11, 2024

Conversation

kingy444
Copy link
Collaborator

Quick PR as alternate solution to #14

This one should set the required defaults for all controller types

Apparently had some bad git log and this has pulled in some previous changes (that are already merged anyway)
im remote at the moment and working via RDP on my phone so hard to get this cleaned up atm 👀

@kingy444
Copy link
Collaborator Author

I think a mix of this PR and the tests from #15 are a good fix

#15 would also fix but the cleanup this one provides cleans up some of the unneeded IF statements

@jahmai
Copy link

jahmai commented Aug 11, 2024

RDP via phone. Kudos on the commitment 👍

@fredrike fredrike merged commit 2344c2e into fredrike:master Aug 11, 2024
2 of 3 checks passed
@fredrike
Copy link
Owner

New version pushed to pypi: https://pypi.org/project/pydaikin/2.13.3/

@joostlek
Copy link

Hold up, let me add to this, creating a mutable object (like a dict in this case) at class level causes the dict to be shared over all instances of that class. This can lead to side effects and I don't think we want that here

@kingy444
Copy link
Collaborator Author

Hold up, let me add to this, creating a mutable object (like a dict in this case) at class level causes the dict to be shared over all instances of that class. This can lead to side effects and I don't think we want that here

Isn't that what caused the issues in 2024.8.1 to start with ?

My understanding is

  • 2024.8.0 broke due to SSL context being required on a particular model
  • 2024.8.1 broke because headers did not exist on all child classes but all child classes shared the same get_response

This PR moved the if headers is None to the definition of the variable and then headers has a value set in the classes that need it

@cremor
Copy link
Collaborator

cremor commented Aug 12, 2024

@kingy444 You are talking about classes in the inheritance hierarchy. But @joostlek is talking about multiple instances of the same class. So in this case multiple different Daikin AC units.
See also https://stackoverflow.com/questions/1680528/how-to-avoid-having-class-data-shared-among-instances

@joostlek
Copy link

Yes, I'm talking about multiple instances of this class, so it would work for users with 1 device, but people with 2 devices will see strange behaviour

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.

6 participants