Skip to content
This repository has been archived by the owner on Jul 23, 2021. It is now read-only.

Remove remotes #238

Merged
merged 13 commits into from
Mar 14, 2020
Merged

Remove remotes #238

merged 13 commits into from
Mar 14, 2020

Conversation

robmarkcole
Copy link
Owner

No description provided.

@robmarkcole
Copy link
Owner Author

@azogue propose we merge this PR and make a seperate one to fix the tests, WDYT?

@azogue
Copy link
Contributor

azogue commented Mar 14, 2020

Whatever you want. But if you can wait some hours, I can clone the branch, fix the tests, make a PR to remove-remotes to clean CI, and then you can merge this into master. IMO is a cleaner approach, but I need some time for RealWord issues :)

@robmarkcole
Copy link
Owner Author

OK sounds good!

azogue and others added 7 commits March 14, 2020 10:49
* Remove remote-platform related
* Complete ZLL sensor tests:
  - Setup binary sensor, adds 1 full ZLL sensor from bridge 1
  - Check generated entity state and attrs
  - Check add_to_hass logic
  - Check behavior for changes in presence or temperature,
  cover strange SML logic around the "changed" attribute
  - Check discovery and setup of a new sensor in bridge 2
  - Cover the error when a data update is received before finishing entity setup
  - Check entity and schedule listener removal on HA stop
so there is only one setup pass here, the initial one with:
```yaml
binary_sensor:
- platform: huesensor
  scan_interval: 2
```

(To achieve 1Hz freq, ommit the `scan_interval` value)
* No need for an integration test without remote,
  moving all to test_binary_sensor
* Just 1 hass fixture with 2 bridges
Fix tests and add warning note on README
@azogue
Copy link
Contributor

azogue commented Mar 14, 2020

It is done :)

You can review and merge #239 here, and then merge this in master.

A doubt though, are you manually generating HACS versions or they are pushed automatically on each master commit? I say this to avoid release this until remotes is done, right?

@azogue
Copy link
Contributor

azogue commented Mar 14, 2020

Wow, that was quick!

@robmarkcole
Copy link
Owner Author

Yes manually generating versions, although I am sure this can be automated..?

@azogue
Copy link
Contributor

azogue commented Mar 14, 2020

I was asking because of #235. I also saw this bug I introduced and I fixed it in the POC I was working on, I forgot to make a PR like #235.

About the HACS releases, I would do:

  • Merging Fix typo in RWL button 1 event #235 into master, and release a last version of 2.x with a note about the next breaking change to come with 3.0 (I think removing the remotes justifies that)
  • Without hurry, merge this one into master and release 3.0 advising of the breaking

although I am sure this can be automated..?

Sure thing, but it is also kind of dangerously

@azogue
Copy link
Contributor

azogue commented Mar 14, 2020

E File "/home/travis/build/robmarkcole/Hue-sensors-HASS/custom_components/huesensor/hue_api_response.py", line 45
391E <<<<<<< HEAD

Looks like a bad rebase you did there

@coveralls
Copy link

Pull Request Test Coverage Report for Build 108

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 97.095%

Totals Coverage Status
Change from base Build 92: 0.6%
Covered Lines: 234
Relevant Lines: 241

💛 - Coveralls

@codecov-io
Copy link

Codecov Report

Merging #238 into master will increase coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   96.52%   97.09%   +0.56%     
==========================================
  Files           6        5       -1     
  Lines         317      241      -76     
==========================================
- Hits          306      234      -72     
+ Misses         11        7       -4
Impacted Files Coverage Δ
custom_components/huesensor/data_manager.py 100% <ø> (+2.25%) ⬆️
custom_components/huesensor/hue_api_response.py 97.56% <100%> (-1.29%) ⬇️
custom_components/huesensor/binary_sensor.py 100% <0%> (+4.54%) ⬆️

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 1b37a68...25da73f. Read the comment docs.

@robmarkcole robmarkcole merged commit 1812adc into master Mar 14, 2020
@robmarkcole robmarkcole deleted the remove-remotes branch March 14, 2020 11:14
@Mariusthvdb
Copy link
Contributor

does this move now force people that want both the sensor, device_tracker and the remote to install both custom integrations, and hence have to install the data mananager and hue api response in both folders?

must say I can't really follow now, since this was move was originally intended to diminish the sources needed, and now seems to cause an increase..

btw, in the readme there is a big warning for causing issues in the core system. Which imho is really an unnecessary disclaimer, since the issues mentioned are not caused by this custom integration, and happen without it all the same.

sorry if I am misunderstanding, but hope this custom integration that was so rock solid and working beautifully independently from the issues in core HA, won't morph into absorbing these issues... really worried here.

@robmarkcole
Copy link
Owner Author

Well you can see the discussion that lead to this move.
TBH I think most people don’t bother reading the docs anyway 😂

@koying
Copy link
Contributor

koying commented Mar 21, 2020

I'm a bit at loss as for the reasoning behind splitting off the remotes to another component, I must say

@Kugelfang666
Copy link

As of quite some versions ago HA also started to support motion sensors. Actually the native support is very good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants