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

Fixing Egardia 'home armed' state not shown correctly. #13335

Merged
merged 4 commits into from Mar 22, 2018
Merged

Fixing Egardia 'home armed' state not shown correctly. #13335

merged 4 commits into from Mar 22, 2018

Conversation

jeroenterheerdt
Copy link
Contributor

@jeroenterheerdt jeroenterheerdt commented Mar 19, 2018

Description:

Fixing Python-egardia bug jeroenterheerdt/python-egardia#17: Home armed state was not showing correctly in Home Assistant. This PR replaces #13309

Checklist:

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

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

@jeroenterheerdt
Copy link
Contributor Author

jeroenterheerdt commented Mar 19, 2018

@MartinHjelmare Travis errors out on pylint with: ERROR: pylint: could not install deps [-r/home/travis/build/home-assistant/home-assistant/requirements_all.txt, -r/home/travis/build/home-assistant/home-assistant/requirements_test.txt, -c/home/travis/build/home-assistant/home-assistant/homeassistant/package_constraints.txt]; v = InvocationError('/usr/bin/env LANG=C.UTF-8 pip install -r/home/travis/build/home-assistant/home-assistant/requirements_all.txt -r/home/travis/build/home-assistant/home-assistant/requirements_test.txt -c/home/travis/build/home-assistant/home-assistant/homeassistant/package_constraints.txt (see /home/travis/build/home-assistant/home-assistant/.tox/pylint/log/pylint-1.log)', 1). The rest runs fine. Can you spot what is wrong this time?

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@@ -1006,9 +1006,11 @@ python_opendata_transport==0.0.3
python_openzwave==0.4.3

# homeassistant.components.egardia
# homeassistant.components.alarm_control_panel.egardia
pythonegardia==1.0.38
Copy link
Member

Choose a reason for hiding this comment

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

This line should be removed. The script should handle that automatically. Did you run the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, I think I know the problem. Will fix tomorrow.

@jeroenterheerdt
Copy link
Contributor Author

Ok, I think it looks better now.

from homeassistant.components.egardia import (
EGARDIA_DEVICE, EGARDIA_SERVER,
REPORT_SERVER_CODES_IGNORE, CONF_REPORT_SERVER_CODES,
CONF_REPORT_SERVER_ENABLED, CONF_REPORT_SERVER_PORT
)
REQUIREMENTS = ['pythonegardia==1.0.38']
REQUIREMENTS = ['pythonegardia==1.0.39']
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed. The alarm platform should depend on the egardia component. Add a DEPENDENCIES list instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I add the DEPENDENCIES list to binary_sensor/egardia.py too? It is also a platform that depends on the egardia component..

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@balloob
Copy link
Member

balloob commented Mar 21, 2018

ok to merge when you ran script/gen_requirements_all

@jeroenterheerdt
Copy link
Contributor Author

@balloob I did and I checked the changes it made manually and they look as expected.

@balloob
Copy link
Member

balloob commented Mar 21, 2018

I doubt you did after making changes… you removed requirement from alarm control panel yet it's still listed as having one in requirements_all.txt

@jeroenterheerdt
Copy link
Contributor Author

@balloob weird, I ran it after the last changes, I am certain. I ran it again and now the changes are pickup up the way they need to be (afaik).

@balloob balloob merged commit 98620d8 into home-assistant:dev Mar 22, 2018
@balloob
Copy link
Member

balloob commented Mar 22, 2018

👍 🐬 🎉

@balloob balloob mentioned this pull request Mar 30, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 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.

None yet

4 participants