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

create separate directory for yeelight #1160

Merged
merged 6 commits into from Oct 19, 2021

Conversation

Kirmas
Copy link
Contributor

@Kirmas Kirmas commented Oct 16, 2021

Based on #1094 (review) but I believe the "moving" PR could be separate. This PR only move the yeelight code to the folder. Next PR will be soon.

@rytilahti could you merge this one?

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2021

Codecov Report

Merging #1160 (136e2e9) into master (c53595f) will increase coverage by 0.50%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
+ Coverage   76.10%   76.61%   +0.50%     
==========================================
  Files          76       77       +1     
  Lines        8927     9155     +228     
  Branches      750      756       +6     
==========================================
+ Hits         6794     7014     +220     
- Misses       1950     1958       +8     
  Partials      183      183              
Impacted Files Coverage Δ
miio/__init__.py 100.00% <100.00%> (ø)
miio/discovery.py 49.33% <100.00%> (+0.68%) ⬆️
miio/integrations/yeelight/__init__.py 88.01% <100.00%> (ø)
miio/integrations/yeelight/tests/test_yeelight.py 96.47% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c53595f...136e2e9. Read the comment docs.

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 17, 2021

Let's move it under integrations/yeelight/ and use absolute imports, this way:

  1. the integrations are clearly separated from the main miio code-base, and
  2. it will be easier to add support for out-of-tree integrations, when all integrations are self-contained

@rytilahti Did I do this correct? Could you provide what else I must change? Maybe move test some how.

@rytilahti
Copy link
Owner

@rytilahti Did I do this correct? Could you provide what else I must change? Maybe move test some how.

Looking good, but yeah, the test should also be moved under tests/ inside the integration directory. Could you try if it still get executed fine after moving it? I think pytest should find it still just fine, and If not, we'll need to figure out how to adjust the test collection. The test itself should probably use a relative import for the module under test, and absolute imports just for modules that are contained under miio or miio.tests.

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 19, 2021

@rytilahti Look to new commit please. Locally test start pytest miio integrations and pytest --cov miio --cov integrations but i`m not shore that this is correct. Could you help me with pytest configuration? Its out of my understanding.

@rytilahti
Copy link
Owner

rytilahti commented Oct 19, 2021

@Kirmas the integrations should be inside the miio package so that it will be accessible so I changed that. Now running pytest miio works and it's finding the test file just fine :-)

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 19, 2021

@Kirmas the integrations should be inside the miio package so that it will be accessible so I changed that. Now running pytest miio works and it's finding the test file just fine :-)

Funny misunderstanding. Is this PR is Ok now? I really want add spec file in the new one. To do this step by step.

@rytilahti
Copy link
Owner

Yes, I think we can merge this now. Thanks for the PR and pushing this onward :-)

@rytilahti rytilahti merged commit 9c04559 into rytilahti:master Oct 19, 2021
@rytilahti rytilahti mentioned this pull request Nov 30, 2021
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.

None yet

3 participants