Skip to content

Handle missing paths in GetDeviceInfo #386

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mr-miles
Copy link

@mr-miles mr-miles commented Feb 1, 2025

TalkTalk (UK) branded F5364 does not recognise the ModelNumber attribute requested in GetDeviceInfo
This causes an exception to be thrown, because GetResponse throws for any ActionError.
End result is that HA cant connect to the router to retrieve the useful info, even though the ModelNumber is not essential info.

Fixed by:

  • Lifting behaviour around ActionErrors to higher level, so caller can react appropriately
  • Specifically avoid throwing exceptions on missing path errors when getting DeviceInfo

There was a TODO around how to deal with multiple ActionErrors, which this PR also addresses - since errors can now be thrown at the point of reading the value from the response if required.

For calls other than GetDeviceInfo, the existing behaviour is retained.

)

@staticmethod
def ThrowIf(response):
Copy link
Owner

Choose a reason for hiding this comment

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

Could you adhere to the PEP8 styleguide? https://peps.python.org/pep-0008/#function-and-variable-names. In Python function names should be lower case with words separated by underscores.

Copy link
Owner

Choose a reason for hiding this comment

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

And good to run pre-commit to see if the linter is happy :-).

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I thought I got all of these but it seems the linter had crashed somewhere and wasn't giving me the full picture, which a reboot has now solved :-/

Copy link
Owner

Choose a reason for hiding this comment

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

No worries. Still plan to move the formatting and linting to ruff and the project to uv, but will need some time first 😃

ex = None

if action_error_desc == XMO_AUTHENTICATION_ERR:
ex = AuthenticationException(action_error)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
ex = AuthenticationException(action_error)
return AuthenticationException(action_error)

My preference would be to not set the ex variable and just return it directly when raised. For all if statements here.

Copy link
Author

Choose a reason for hiding this comment

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

Mine too ... I thought I saw "too many return statements" in the linter output and just complied. But I think my setup was skewy for linting so I've changed it back to see if it still complains

Copy link
Author

Choose a reason for hiding this comment

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

yeah its not happy about R0911 (too-many-returns) now, not sure which way you want to go?

Copy link
Owner

Choose a reason for hiding this comment

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

We can add an ignore for now, or change the linting preferences.

Copy link
Author

Choose a reason for hiding this comment

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

now disabled by comment

Copy link
Author

Choose a reason for hiding this comment

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

@iMicknl - any chance you can kick off the workflow again? sorry about the chain of back-and-forth

@mr-miles mr-miles force-pushed the talktalk branch 2 times, most recently from ac9b48f to 7d1b9c6 Compare February 3, 2025 16:35
Lift behaviour to higher level so caller can specify the behaviour

Specifically to avoid throwing exceptions on missing path errors when getting DeviceInfo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants