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

Load requirements and dependencies from manifests. Fallback to current REQUIREMENTS and DEPENDENCIES #22717

Merged
merged 57 commits into from
Apr 11, 2019

Conversation

rohankapoorcom
Copy link
Member

@rohankapoorcom rohankapoorcom commented Apr 4, 2019

Breaking Change:

To developers: Setup entity platform will set up its component now despite DEPENDENCIES.

For example, if you have an integration smart_home, it has some connection management code in smart_home/__init__.py and it provides light and sensor. In smart_home/light.py, it has DEPENDENCIES=['smart_home'], it will use smart_home/__init__.py to manage its connection. So when light domain set up its platform, it will load smart_home/light.py, then as the dependencies's require, smart_home/__init__.py will be load and its async_setup() method will be called. The process is not changed, however, such dependencies pointed to its own component is not required anymore, because we will load and set up the smart_home component as long as you set up smart_home/light.py no matter DEPENDENCIES exists or not.

On the other hand, let's say smart_home/sensor.py does not rely on smart_home/__init__.py, so there is no DEPENDENCIES in smart_home/sensor.py. After this PR, the behavior changed, we will load and set up the smart_home component as long as you set up smart_home/sensor.py

Such behavior change may break your component. As far as I know, following component will have problem after this PR merged

  • demo
  • google
  • telegram

Description:

Load requirements and dependencies from manifests (if present). If not, fallback to the current approach of reading REQUIREMENTS and DEPENDENCIES from the module attribute.

Related issue (if applicable): relates to #22700

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@rohankapoorcom rohankapoorcom requested a review from a team as a code owner April 4, 2019 08:24
@ghost ghost assigned rohankapoorcom Apr 4, 2019
@ghost ghost added the in progress label Apr 4, 2019
@balloob balloob mentioned this pull request Apr 4, 2019
13 tasks
homeassistant/setup.py Outdated Show resolved Hide resolved
homeassistant/setup.py Outdated Show resolved Hide resolved
@rohankapoorcom
Copy link
Member Author

Will fix in the evening. Thanks for the feedback.

@awarecan
Copy link
Contributor

Add tests/common::mock_entity_platform() method, can be use to replace loader.set_component

-        loader.set_component(self.hass, 'test_domain.platform', platform)
+        mock_entity_platform(self.hass, 'test_domain.platform', platform)

homeassistant/setup.py Outdated Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
@awarecan
Copy link
Contributor

@balloob I have to go, won't check in code for this PR today. Feel free to change whatever need

@balloob balloob added this to the 0.92.0 milestone Apr 10, 2019
Temp workaround to unblock CI tests
@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@balloob
Copy link
Member

balloob commented Apr 11, 2019

I think I got last one fixed. When it's green, let's merge

@awarecan
Copy link
Contributor

"fixed" google tts.

Now load google tts platform will setup google component as well. I add a shortcut in google component to allow test pass. However it may break the google component, e.g. google calendar and tts cannot configured together. We need reorg google tts to a separate folder, such as google_tts. If we want to merge several google_* component then we need do it right, not a simple git mv.

@awarecan awarecan merged commit 6ba9ccf into dev Apr 11, 2019
@ghost ghost removed the in progress label Apr 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the load-requirements-manifest branch April 11, 2019 08:26
@balloob balloob removed this from the 0.92.0 milestone Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants