Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Update __init__.py #3

Merged
merged 8 commits into from
Apr 5, 2020
Merged

Update __init__.py #3

merged 8 commits into from
Apr 5, 2020

Conversation

ejonesnospam
Copy link
Contributor

Adds media controls. Improves error control for send_key commands

Adds media controls. Improves error control for send_key commands
try:
self._call_service(service="remote_service", action="sendkey", payload=keyname)
result = True
except json.JSONDecodeError:
Copy link
Owner

Choose a reason for hiding this comment

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

I am a little confused by this, what is raising the JSONDecodeError? Is this something that can be fixed elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, well. I have absolutely no idea where that came from... lol.
That line should just be "except:"

Copy link
Owner

Choose a reason for hiding this comment

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

What exception is getting raised there? It would be better to scope it down a little, a bare except will catch system level events like KeyboardInterrupt and SystemExit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd have to defer to you here, as my best practices knowledge on exception handling is limited.

Copy link
Owner

Choose a reason for hiding this comment

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

Generally I try to propagate exceptions as much as possible to the user - unless the exceptions are errors due to the internal code rather than a device malfunction or user error.

If this is an internal code error it would be helpful to know what causes it so that we could perhaps change the internal workings to function better, and limit the scope to capture just the exception(s) that are problematic. Are there any specific exceptions that you have had? Do you have the traceback for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I experienced no exceptions in my development. My thought process was to always return a bool value, based on success of send_key command. Any exception failure would be captured and appropriate return value sent.

ejonesnospam and others added 2 commits March 12, 2020 17:57
Correction to exception response.
@newAM
Copy link
Owner

newAM commented Mar 22, 2020

I committed some changed to your branch - I narrowed the scope of the exception catching for JSON to just the JSONDecodeError, and removed the catch for _send_key exceptions.

I would prefer that _send_key propagate exceptions up to the user so that in case of problems that could be fixed by the user they get all the information instead of a True / False.

Let me know what you think!

Sorry for the slow replies, the coronavirus has made my life a little crazy recently.

@ejonesnospam
Copy link
Contributor Author

ejonesnospam commented Mar 24, 2020

No reason to apologize, if your community is not at or near chaos right now, you must be living in space.

I can totally agree understand that _send_key may should remain as is. After looking at again, I agree the exceptions are better if propagated up.
To allow more functionality, would it be better to include a template public command in the api, like below? Since the MQTT interface is not really supported publicly by Hisense, any future firmware change could alter or add more commands, and a fix may not require an API change (if lucky).
def send_key_template(self, keyname: str):
"""
Sends a keypress to the TV, as if it had been pressed on the IR remote.
Args:
keyname: Name of the key press to send.
"""
self._call_service(service="remote_service", action="sendkey", payload=keyname)

@newAM
Copy link
Owner

newAM commented Mar 28, 2020

Nothing is every truly private in python so someone could just overwrite the private _send_key, maybe we should make it public?

class MyTv(HisenseTv):
    def _send_key(self, keyname: str):
        self._call_service(service="remote_service", action="new_sendkey_action", payload=keyname)

updates send_key to public
Added 'ok' to list of commands
adds ok,media control commands to usage
renames ff & rew
correction to usage for controls
@ejonesnospam
Copy link
Contributor Author

Yes, I feel it should be public. Also updated README for new media control commands. How do you like the nomenclature: "fast_forward" , "rewind", "volume_up" ?

@newAM
Copy link
Owner

newAM commented Apr 5, 2020

Looks good to me! Should I merge now?

@ejonesnospam
Copy link
Contributor Author

ejonesnospam commented Apr 5, 2020 via email

@newAM newAM merged commit f5b0f10 into newAM:master Apr 5, 2020
@ejonesnospam
Copy link
Contributor Author

Can you please update PyPi so I can download version 0.0.7

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants