Skip to content

Async version for asuswrt#17692

Merged
balloob merged 2 commits intohome-assistant:devfrom
kennedyshead:async_asuswrt
Oct 23, 2018
Merged

Async version for asuswrt#17692
balloob merged 2 commits intohome-assistant:devfrom
kennedyshead:async_asuswrt

Conversation

@kennedyshead
Copy link
Copy Markdown
Contributor

@kennedyshead kennedyshead commented Oct 22, 2018

Description:

This is a new async-version for asuswrt.

It will hopefully fix a lot of other errors with duplicate ssh-channels as well

Related issue (if applicable):
fixes #13723, fixes #17063 fixes #16887 fixes #13757 fixes #13351 fixes #10568

Checklist:

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

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 22, 2018

Could you extract it into a small lib that we can depend on instead?

@kennedyshead
Copy link
Copy Markdown
Contributor Author

Yes I might be able to, do you mean a smaller ssh-lib or a small asuswrt-lib? @balloob

from homeassistant.components.device_tracker.asuswrt import (
CONF_PROTOCOL, CONF_MODE, CONF_PUB_KEY, DOMAIN, _ARP_REGEX,
CONF_PORT, PLATFORM_SCHEMA, Device, get_scanner, AsusWrtDeviceScanner,
CONF_PORT, PLATFORM_SCHEMA, Device, async_get_scanner, AsusWrtDeviceScanner,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Copy link
Copy Markdown
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.

Awesome!

@@ -152,237 +100,9 @@ def _update_info(self):
return False
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.

Nothing is checking this return value.

data = self.get_asuswrt_data()
data = await self.connection.async_get_connected_devices()
if not data:
return False
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.

See above.

return False

self.last_results = data
return True
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.

See above.

@kennedyshead
Copy link
Copy Markdown
Contributor Author

I don't think that redundant checks if data is returned is needed, the simpler the better ;)

Copy link
Copy Markdown
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.

Exactly! 👍

self.username, self.password, self.ssh_key,
self.mode, self.require_ip)

self.last_results = {}
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.

Do we need to reset this here? It's already initialized in init.

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.

You are right, that is just sloppy coding

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 was left.

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.

🙈

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.

oops

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.

@kennedyshead
Copy link
Copy Markdown
Contributor Author

I added all issues that are related to this as a possible fix for it, the lib issues should not be here anyway ;)

self.last_results = {}
self.success_init = False
self.connection = AsusWrt(self.host, self.port,
False if self.protocol == 'ssh' else True,
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.

LOL! I broke my own PEP request!!!

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 23, 2018

amazing!

@balloob balloob merged commit 277a9a3 into home-assistant:dev Oct 23, 2018
@ghost ghost removed the in progress label Oct 23, 2018
@kennedyshead kennedyshead deleted the async_asuswrt branch October 23, 2018 11:06
@balloob balloob mentioned this pull request Nov 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

7 participants