Skip to content

Unload scripts and unsubscribe from event listeners during entry unload #53

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

Merged
merged 3 commits into from
Oct 23, 2020

Conversation

raman325
Copy link
Contributor

Implements changes discussed in #42

@craigbarratt
Copy link
Member

This definitely look like the right approach. I'm busy for at least the next day, so I'll check it out further then.

@raman325
Copy link
Contributor Author

Just to add, I added a call to Function.reaper_stop() in async_unload_entry to prevent the following error from occurring due to the test:

run_coro: got exception Traceback (most recent call last):
  File "/Users/raman/projects/pyscript/custom_components/pyscript/function.py", line 234, in run_coro
    await coro
RuntimeError: coroutine ignored GeneratorExit

not sure if this call belongs in async_unload_entry or in unload_scripts so I used my best judgement

@craigbarratt craigbarratt merged commit 84f55e6 into custom-components:master Oct 23, 2020
@craigbarratt
Copy link
Member

Thanks. I'll test it after I merge.

@raman325
Copy link
Contributor Author

raman325 commented Oct 23, 2020

Sounds good. One thing I realized that may be worth pointing out is that performing a reload through the Integrations UI will not reload the configuration.yaml information, it will just reload the integration and the data that already exists in the config entry. If it is desirable to have both reloads (service and integrations UI) work the same way in that if configuration.yaml is updated, pyscript sees the update, I can look at how to achieve that.

@raman325 raman325 deleted the unload_scripts branch October 23, 2020 03:32
@raman325
Copy link
Contributor Author

Doing what I suggested above would be relatively easy I think, we just need to copy this logic: https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/__init__.py#L103-L114 and add it here: https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/__init__.py#L90

This also begs the question, if we are trying to mimick the reload service, should the service be retired and instead direct users to use the Integrations UI? The advantage of having a service is that it can be called from an automation, whereas a UI reload can't

@craigbarratt
Copy link
Member

Great - that's basically what I did in 0d4c5a6. UI reload and the reload service should be the same now. I confirmed yaml config updates work in both cases, and task cleanup seems correct now too.

Yes, I'd like to keep the service so you can call it from Jupyter or an automation, as you point out.

@raman325
Copy link
Contributor Author

Nice! 👍

@raman325
Copy link
Contributor Author

raman325 commented Oct 24, 2020

By the way, I'm sorry I didn't realize the true complexity of trying to add config flow support, I naively thought it would be simpler. But the end result is pretty cool!

And if Home Assistant adds support more complex configurations through the UI, eg to support 'apps' we should be able to simplify some of the config flow logic in the future

@dlashua
Copy link
Contributor

dlashua commented Oct 24, 2020

I would love to see the ability to add an "app" (even if I have to plug the YAML into a form input) from the UI. I, personally, won't use it much. But, I know that would make it a lot easier for some other users.

@craigbarratt
Copy link
Member

@raman325 - no problem at all. I'm a beginner on HASS so it's helpful for me to understand more of its features. Thanks for your diligence and quick follow-up on questions or tweaks to the code. Your additions turned out nicely.

Yes, if apps in pyscript become popular, we could figure out a way to add config flow support for them as you suggest. Perhaps we would have some way for a pyscript app to define its schema (eg, setting a global variable like an integration does), and that could be used in config flow and for error checking. But I don't think it's worth doing that yet.

@raman325
Copy link
Contributor Author

👍 I wouldn't call myself an HA expert but I'm a relatively frequent contributor so happy to help where I can.

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