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

Removing asyncio.coroutine syntax from HASS core #12509

Merged
merged 14 commits into from
Feb 25, 2018

Conversation

Julius2342
Copy link
Contributor

@Julius2342 Julius2342 commented Feb 18, 2018

Description

Changed old asyncio.coroutine/ yield from syntax to async def / await syntax within core libraries.

Current state

Runs here on my home on production without problems.

13 unit tests fail.

    3299 passed
      13 failed
         - tests/test_bootstrap.py:43 test_home_assistant_core_config_validation[pyloop]
         - tests/components/emulated_hue/test_hue_api.py:219 test_put_light_state[pyloop]
         - tests/components/emulated_hue/test_hue_api.py:346 test_put_with_form_urlencoded_content_type[pyloop]
         - tests/components/mqtt/test_discovery.py:24 test_invalid_topic[pyloop]
         - tests/components/mqtt/test_discovery.py:37 test_invalid_json[pyloop]
         - tests/components/mqtt/test_discovery.py:51 test_only_valid_components[pyloop]
         - tests/components/notify/test_apns.py:357 TestApns.test_disable_when_unregistered
         - tests/components/sensor/test_coinmarketcap.py:31 TestCoinMarketCapSensor.test_setup
         - tests/helpers/test_discovery.py:27 TestHelpersDiscovery.test_listen
         - tests/scripts/test_check_config.py:120 TestCheckConfig.test_component_platform_not_found
         - tests/scripts/test_check_config.py:73 TestCheckConfig.test_config_component_platform_fail_validation
         - tests/scripts/test_check_config.py:57 TestCheckConfig.test_config_platform_valid
         - tests/util/test_logging.py:9 test_sensitive_data_filter

@balloob
Copy link
Member

balloob commented Feb 18, 2018

For the test that failed on config entries, try swapping the two decorators around in config/config_entries.py line 100 (or convert to async/await)

@Julius2342 Julius2342 changed the title [WIP] Removing asyncio.coroutine syntax from HASS core Removing asyncio.coroutine syntax from HASS core Feb 23, 2018
@@ -569,8 +568,7 @@ def async_will_remove_from_hass(self):
self._async_unsub_state_changed()
self._async_unsub_state_changed = None

@asyncio.coroutine
def _async_state_changed_listener(self, entity_id, old_state, new_state):
async def _async_state_changed_listener(self, entity_id, old_state, new_state):

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

@@ -569,8 +568,7 @@ def async_will_remove_from_hass(self):
self._async_unsub_state_changed()
self._async_unsub_state_changed = None

@asyncio.coroutine
def _async_state_changed_listener(self, entity_id, old_state, new_state):
async def _async_state_changed_listener(self, entity_id, old_state, new_state):

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

@Julius2342 Julius2342 requested a review from a team as a code owner February 23, 2018 21:27
@balloob
Copy link
Member

balloob commented Feb 23, 2018

Please limit changes to code that is covered by tests (but do not update the tests in the same PR).

@MartinHjelmare
Copy link
Member

This is not only core changes. Please update PR title.

@Julius2342 Julius2342 changed the title Removing asyncio.coroutine syntax from HASS core Removing asyncio.coroutine syntax from HASS core a d Feb 24, 2018
@Julius2342 Julius2342 changed the title Removing asyncio.coroutine syntax from HASS core a d Removing asyncio.coroutine syntax from HASS core and some places within components Feb 24, 2018
@Julius2342
Copy link
Contributor Author

@balloob : how should I handle linting problems within components? (pylint complained at various places that a function from core is (no longer) iterable? I made the functions async where the complexity is low and added a pylint ignore where I saw the risk of breaking something.

I'm also not sure how to handle failed unit tests...

@balloob
Copy link
Member

balloob commented Feb 24, 2018

You will have to fix the unit tests..

@Julius2342 Julius2342 changed the title Removing asyncio.coroutine syntax from HASS core and some places within components Removing asyncio.coroutine syntax from HASS core Feb 24, 2018
@RequestDataValidator(vol.Schema({
vol.Required('domain'): str,
}))
@asyncio.coroutine
Copy link
Member

Choose a reason for hiding this comment

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

Since this is covered by tests, we could also just add async keyword to function.

@@ -24,9 +23,9 @@ def async_process_requirements(hass, name, requirements):
pip_install = partial(pkg_util.install_package,
**pip_kwargs(hass.config.config_dir))

with (yield from pip_lock):
with (await pip_lock):
Copy link
Member

Choose a reason for hiding this comment

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

I think that this can be replaced with async with pip_lock:

@balloob
Copy link
Member

balloob commented Feb 24, 2018

Okay I addressed my own comment and fixed the merge conflict, however the logging test still fails. I am unable to repro it locally on Python 3.5.3 🤔 This is why smaller PRs will help, makes it easy to see why something is causing an error.

@Julius2342
Copy link
Contributor Author

Julius2342 commented Feb 25, 2018

regarding util/test_logging.py :

  • it is reproducable locally when running all tests, but not when running the single test
  • it already fails when initializing logging_util.HideSensitiveDataFilter

I fixed this test by doing:

diff --git a/tests/util/test_logging.py b/tests/util/test_logging.py
index 94c8568..c67b2ae 100644
--- a/tests/util/test_logging.py
+++ b/tests/util/test_logging.py
@@ -6,7 +6,6 @@ import threading
 import homeassistant.util.logging as logging_util
 
 
-@asyncio.coroutine
 def test_sensitive_data_filter():
     """Test the logging sensitive data filter."""
     log_filter = logging_util.HideSensitiveDataFilter('mock_sensitive')

(and JFTR: the flaky test_pilight.py test is already flaky on current dev )

@balloob balloob merged commit 16cb738 into home-assistant:dev Feb 25, 2018
@Julius2342 Julius2342 deleted the async-core branch February 25, 2018 19:52
@balloob balloob mentioned this pull request Mar 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.

5 participants