Skip to content

Fix position of add_entities of binary sensor#22866

Merged
fabaff merged 5 commits into
home-assistant:devfrom
AZDane:dev
Apr 8, 2019
Merged

Fix position of add_entities of binary sensor#22866
fabaff merged 5 commits into
home-assistant:devfrom
AZDane:dev

Conversation

@AZDane
Copy link
Copy Markdown
Contributor

@AZDane AZDane commented Apr 7, 2019

Description: Add_entities is wrongly placed inside loop, so it ads the same sensor multiple times creating errors during HA restart.

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.

@ghost ghost added the in progress label Apr 7, 2019
Copy link
Copy Markdown
Contributor

@bachya bachya left a comment

Choose a reason for hiding this comment

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

Can be merged when build passes.

@fabaff fabaff changed the title Bugfix - binary_sensor.py Fix position of add_entities of binary sensor Apr 7, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2019

Codecov Report

Merging #22866 into dev will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22866      +/-   ##
==========================================
+ Coverage   93.83%   93.83%   +<.01%     
==========================================
  Files         448      448              
  Lines       36528    36528              
==========================================
+ Hits        34275    34277       +2     
+ Misses       2253     2251       -2
Impacted Files Coverage Δ
homeassistant/components/uk_transport/sensor.py 93.43% <0%> (-0.73%) ⬇️
...ssistant/components/islamic_prayer_times/sensor.py 97.89% <0%> (+3.15%) ⬆️

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 e407226...90a89d5. Read the comment docs.

@AZDane
Copy link
Copy Markdown
Contributor Author

AZDane commented Apr 8, 2019

I actually wanted to ad the second commit as a different pull request, but ended up commiting it instead, so I have updated the description. Hope that works! Thanks guys!

@syssi
Copy link
Copy Markdown
Member

syssi commented Apr 8, 2019

If you remove the new feature this fix can be merged into 0.91.2.

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_HOST, default=DEFAULT_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_CODE, 'code validation'): cv.string,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a const, too.

Copy link
Copy Markdown
Contributor Author

@AZDane AZDane Apr 8, 2019

Choose a reason for hiding this comment

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

It is if defined in configuration.yaml. If not it will be None. I was trying to make it as much as the Manual Alarm Panel as possible:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/manual/alarm_control_panel.py
see ln 77
is that in error too?

Are you suggesting adding:

Suggested change
vol.Optional(CONF_CODE, 'code validation'): cv.string,
DEFAULT_CODE = None
vol.Optional(CONF_CODE, default=DEFAULT_CODE): cv.string,

Obviously shouldn't be committed like that, but at the appropriate places.

How would I go about removing the new feature fro the PR? Just recommit with the original file without changes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are mixing things up here. The string "code validation" at alarm_control_panel.py is the identifier between the different vol.Exclusive options. What about:

vol.Optional(CONF_CODE): cv.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. You can recommit the initial file and check the diff here afterwards.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok!! Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have made a commit with the original file and updated the comment to only address the fix. Does it look ok?

Removed added feature and sticking to bugfix
@AZDane
Copy link
Copy Markdown
Contributor Author

AZDane commented Apr 8, 2019

Removed the added feature and now back to just the one fix. Hopefully it can be added into 0.91.2.

@fabaff fabaff merged commit 45a4359 into home-assistant:dev Apr 8, 2019
@ghost ghost removed the in progress label Apr 8, 2019
@awarecan awarecan added this to the 0.91.3 milestone Apr 8, 2019
@awarecan
Copy link
Copy Markdown
Contributor

awarecan commented Apr 8, 2019

You missed the train. I created 0.91.3 milestone and assigned.

pvizeli pushed a commit that referenced this pull request Apr 10, 2019
* Bugfix - binary_sensor.py

* Added features to Concord232 Alarm Panel

* Added New Line End Of File

* Deleted Whitespace

* Back to original

Removed added feature and sticking to bugfix
@pvizeli pvizeli mentioned this pull request Apr 10, 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.

7 participants