Skip to content
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

Bugfix/skills/browser #114

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

KingofGnome
Copy link

Hi,
as discussed in #112, I reworked the open_in_youtube-skill.
Its still a requests.get without using any api or ned of any api-key.

Instead of using bs4, i regex-search for the javascript initialization-data, transform the json-string and use glom to extract the deeply nested video url as well as video title.

  • added glom==20.11.0 to requirements.txt
  • added my settings to the pullrequest :/

Other changes:

  • removed if reg_ex so Jarvis will not silently fail/get "stuck" with the INFO - Executing skill open_in_youtube -message if one just say 'play', cause it looks like a freeze

added error-message if no results were found (missing url)
undid test-format for requests
Copy link
Contributor

@89Q12 89Q12 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 to me!
But could you please change the settings back to the default settings and could you change line 90 to the same as line 97 so that we have consistent error handling?
Would also directly show the user if they pronounced it wrong or so :)
Otherwise thanks for fixing!

@KingofGnome
Copy link
Author

Looks good to me!
But could you please change the settings back to the default settings and could you change line 90 to the same as line 97 so that we have consistent error handling?
Would also directly show the user if they pronounced it wrong or so :)
Otherwise thanks for fixing!

Sure, ill get on it the next days.

@KingofGnome
Copy link
Author

@11Tuvork28

could you change line 90 to the same as line 97 so that we have consistent error handling? Would also directly show the user if they pronounced it wrong or so :) Otherwise thanks for fixing!

I'm actually not quite sure what do you mean.
Currently i'll raise an exception with the error message: 'No YT-video was found for '{search_text}'.' That will get catched by the existing try-catch and print out on the console + error logs:
2021-10-26 21:11:29,186 - root - ERROR - Error with the execution of skill with message No YT-video was found for 'asdfiou90a78 9078asdf'.
while JARVIS will always says I can't find what do you want in Youtube.. if anything goes wrong.

It might not be best practice to intentionally throw exceptions, however if i just debug/error-log into the console we don't have JARVIS output (meaning no voice respond to your input).
To do that we needed to duplicate the I can't find what do you want in Youtube.. line.

Or do you want something like:

if not yt_results:
     cls.response(f"I could not find a video searching for {search_text}")

and completely avoid every exception?
I actually like last last idea the most.

I intentionally tried to keep the error logging as it was before (besides the ':', removed them already) that's the reason i went with trowing a custom Exception-Message:

image

@89Q12
Copy link
Contributor

89Q12 commented Oct 27, 2021

Yeah sorry should have clarified that, my bad!
What I meant was that it should be

if not yt_results:
     cls.response(f"I could not find a video searching for {search_text}")

So basically what you already had in mind.
As that would give the user more information on what was wrong.

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.

None yet

2 participants