Skip to content

Revert custom component loading logic#14327

Merged
pvizeli merged 4 commits intodevfrom
revert-custom-component-loading
May 7, 2018
Merged

Revert custom component loading logic#14327
pvizeli merged 4 commits intodevfrom
revert-custom-component-loading

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented May 7, 2018

Description:

Revert the loading logic refactor for custom components introduced in #14211. Keeping the cleanup 👍

Added some tests to make sure that future refactors are aware of certain functionality that will break.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

@balloob balloob requested a review from a team as a code owner May 7, 2018 03:32
@balloob balloob added this to the 0.69 milestone May 7, 2018
}
}))
assert handle_config[notify.DOMAIN]
), patch('homeassistant.components.notify.file.os.stat') as mock_st, \
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mocking os.stat while imports were being done was messing with the tests.

@pvizeli pvizeli merged commit 5c95c53 into dev May 7, 2018
@pvizeli pvizeli deleted the revert-custom-component-loading branch May 7, 2018 09:25
@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented May 7, 2018

Some problems are only vissible in real world scenario :) But code have looking okay before and now it is better.

@MartinHjelmare
Copy link
Copy Markdown
Member

Why didn't the new logic work?

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented May 7, 2018

The first cleanup looks amazing, but with last modification it going into direction to become the label "uggly". The old was a bit cleaner at this part. The last PR they fix the naming problem, was hard on limit for a core PR.

I approved the last one because some times it need tests on real world and the code/idea need some grow time until he is perfect.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented May 7, 2018

@MartinHjelmare the problem was that we didn't provide a "full" Python import environment by importing the files directly. That means that things like from .const import bla broke (which I am fine with) but also I couldn't get it processed under the right name. So instead of custom_components.bla it would get the name bla. This probably had a bunch of other side effects that we didn't even notice yet. PyCon is around the corner, I didn't feel like digging deep into this right now.

The right solution would be to be able to mount a folder in the Python search path but only be able to whitelist 1 subfolder as being allowed to import from.

balloob added a commit that referenced this pull request May 7, 2018
* Revert custom component loading logic

* Lint

* Fix tests

* Guard for infinite inserts into sys.path
This was referenced May 7, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants