-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: main
Are you sure you want to change the base?
Conversation
) | ||
|
||
@staticmethod | ||
def ThrowIf(response): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-).
There was a problem hiding this comment.
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 :-/
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now disabled by comment
There was a problem hiding this comment.
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
ac9b48f
to
7d1b9c6
Compare
Lift behaviour to higher level so caller can specify the behaviour Specifically to avoid throwing exceptions on missing path errors when getting DeviceInfo
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:
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.