Skip to content

Add Bridge module to AsusWRT#84152

Merged
bieniu merged 5 commits into
home-assistant:devfrom
ollo69:asuswrt-add-bridge
Jul 1, 2023
Merged

Add Bridge module to AsusWRT#84152
bieniu merged 5 commits into
home-assistant:devfrom
ollo69:asuswrt-add-bridge

Conversation

@ollo69
Copy link
Copy Markdown
Contributor

@ollo69 ollo69 commented Dec 17, 2022

Proposed change

I'm trying to split PR #71899 in multiple smaller parts.
This first PR just introduces the bridge module, that implement an abstract AsusWrtBridge class that than is derived to use a specific library (like SamsungTV integration bridge module).
With this PR there are no changes in integration functionality.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @kennedyshead, mind taking a look at this pull request as it has been labeled with an integration (asuswrt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of asuswrt can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign asuswrt Removes the current integration label and assignees on the issue, add the integration domain after the command.

@PeteRager
Copy link
Copy Markdown
Contributor

What help can I provide to get your updated code acceptable to merge?

@ollo69
Copy link
Copy Markdown
Contributor Author

ollo69 commented Feb 1, 2023

The point is have a ha team member that review the PR. This is up to them.

@ollo69 ollo69 force-pushed the asuswrt-add-bridge branch 4 times, most recently from 99056bc to 9a97685 Compare February 8, 2023 08:57
@ollo69 ollo69 mentioned this pull request Apr 11, 2023
19 tasks
@ollo69 ollo69 force-pushed the asuswrt-add-bridge branch from 9a97685 to 4fe7262 Compare April 18, 2023 21:29
@ollo69 ollo69 force-pushed the asuswrt-add-bridge branch from 4fe7262 to 3a6bd41 Compare May 7, 2023 17:16
@ollo69 ollo69 force-pushed the asuswrt-add-bridge branch from 3a6bd41 to 2ccca8c Compare June 25, 2023 11:44
Comment thread homeassistant/components/asuswrt/bridge.py Outdated
Comment thread homeassistant/components/asuswrt/config_flow.py Outdated
@bieniu

This comment was marked as resolved.

@ollo69
Copy link
Copy Markdown
Contributor Author

ollo69 commented Jun 29, 2023

I caught two unhandled exception as I tested the change.

There are no change in the coordinator function here. This would happen here as in the version without bridge and exception is managed by coordinator. Do you mean that I should catch this exception and raise and UpdateError?

@bieniu
Copy link
Copy Markdown
Member

bieniu commented Jun 29, 2023

Do you mean that I should catch this exception and raise and UpdateError?

Yes, those exceptions should end with UpdateError but we don't have to fix this in this PR. This was a side note.

Comment thread homeassistant/components/asuswrt/bridge.py Outdated
Comment thread homeassistant/components/asuswrt/bridge.py Outdated
Comment thread homeassistant/components/asuswrt/bridge.py Outdated
Copy link
Copy Markdown
Member

@bieniu bieniu left a comment

Choose a reason for hiding this comment

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

Please add IndexError to these two methods. This will fix the two exceptions I got a few days ago.

Comment thread homeassistant/components/asuswrt/bridge.py Outdated
Comment thread homeassistant/components/asuswrt/bridge.py Outdated
@home-assistant home-assistant Bot marked this pull request as draft July 1, 2023 10:09
@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Jul 1, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@bieniu bieniu added the smash Indicator this PR is close to finish for merging or closing label Jul 1, 2023
@ollo69
Copy link
Copy Markdown
Contributor Author

ollo69 commented Jul 1, 2023

Please add IndexError to these two methods. This will fix the two exceptions I got a few days ago.

Added also to _get_bytes that could have same issue

@ollo69 ollo69 requested a review from bieniu July 1, 2023 10:32
@ollo69 ollo69 marked this pull request as ready for review July 1, 2023 10:35
Copy link
Copy Markdown
Member

@bieniu bieniu 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 👍 Tested with RT-AX53U and works as intended.

@bieniu bieniu merged commit 8108a0f into home-assistant:dev Jul 1, 2023
@bieniu
Copy link
Copy Markdown
Member

bieniu commented Jul 1, 2023

@ollo69 Please tag me in the next PRs you open to implement http communication and I'll try to help with the review.

@ollo69
Copy link
Copy Markdown
Contributor Author

ollo69 commented Jul 1, 2023

@ollo69 Please tag me in the next PRs you open to implement http communication and I'll try to help with the review.

Ok, thanks. I'll start working on this shortly.

@ollo69 ollo69 deleted the asuswrt-add-bridge branch July 1, 2023 11:59
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 2, 2023
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.

3 participants