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

Do not call update() in constructor #8878

Merged
merged 2 commits into from Aug 8, 2017

Conversation

Projects
None yet
4 participants
@fabaff
Copy link
Member

commented Aug 7, 2017

Description:

Do not call update() in constructor.

Second round...first round #8859

Checklist:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@balloobbot balloobbot added the platform label Aug 7, 2017

@fabaff fabaff force-pushed the update-constructor2 branch Aug 7, 2017

@fabaff fabaff force-pushed the update-constructor2 branch to 5ad95ed Aug 7, 2017

@fabaff fabaff added cla-recheck and removed cla-signed labels Aug 7, 2017

@homeassistant homeassistant added cla-signed and removed cla-recheck labels Aug 7, 2017

@fabaff

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

I will merge this. This will give me more time to get feedback if something is broken till the release on Saturday.

@fabaff fabaff merged commit f513f62 into dev Aug 8, 2017

5 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0002%) to 93.766%
Details
hound No violations found. Woof!

@fabaff fabaff deleted the update-constructor2 branch Aug 8, 2017

@fabaff fabaff referenced this pull request Aug 12, 2017

Merged

0.51 #8919

dethpickle added a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Do not call update() in constructor (home-assistant#8878)
* Do not call update() in constructor

* Fix lint issues
@dethpickle

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2017

Hi, @fabaff. Sorry I missed the changes coming. #8878 did break the digitalloggers switch.

Maybe you can help me understand what you're trying to avoid in "Do not call update() in constructor", so I can fix it correctly. I modeled off some other switch where device attributes are set in the update, but that initial set of statuses can either duplicate that code or just call self.update().

You seem to want to be specifically avoiding that. I'm normally trying to avoid duplicating code, but this just suggests I'm doing something wrong in the first place.

Ultimately, the switch broke because this is one of those appliances with 8 switches on a parent switch. Discovery has to start with the parent and work it's way down to the individual switch. You had replaced the parent's __init__ self.update() with a self.statuslocal = None, when that was when it was supposed to get all it's initial data for setting up the eight individual switches. I can replace it with a self.statuslocal = self._device.statuslist(), which is basically all that happens in the update() anyway. It just needs it's initial data from somewhere before it gets around to registering the children with HA.

Even if update is called again, the @Throttle around the parent's update should make it continue on with cached data, right? Or am I mistaking how Throttle works?

Regardless, I have it working again in my environment by duping the code that would be in the update up into the init. But please let me know what you were aiming for so I can make sure I fix it right before I try to submit a pull request. I'm probably the only one using this platform, so it's probably not a big rush.

Thanks!

Dave

@dale3h dale3h referenced this pull request Aug 28, 2017

Merged

Fix and optimize digitalloggers platform #9203

0 of 1 task complete

@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.