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

Adding a event listener to respond with text without speaking #38

Open
wants to merge 2 commits into
base: 20.08
Choose a base branch
from

Conversation

hughwilliams94
Copy link

I was wanting to use information from WolframAlpha in another skill without mycroft speaking the results so I added a listener for wolfram.unspoken.request messages which triggers the emit_text_result method and a wolfram.unspoken.response message is then sent with the result.

I had to add the query_to_string method because the query method kept failing because wolfram.Result() did not like the output of 'BytesIO(data.content)`.

This is my first time contributing, and I was a little unsure of how the Api inheritance was working so I would love some feedback on how it can be improved.

@Y0ngg4n
Copy link

Y0ngg4n commented Sep 18, 2020

@hughwilliams94 Looks good 👍

@krisgesling
Copy link
Contributor

Hey Hugh,

Thanks for the contribution, it's a great idea and would be a good use case for the Skill API however that isn't yet included in core. So this seems like a valid work around until then.

I would suggest we prepend skill to the Message names eg something like skill.wolfram.unspoken.request just to follow our namespacing convention.

However it's also not immediately apparent to me why one is "spoken" vs "unspoken". Perhaps the "spoken" method was intended to emulate the API endpoint. It seems that the differentiation is really localization, is that right? I.e. spoken requires geocoordinates and unit of measure, whilst this is a query only request.

Given this will be an externally facing interface (from this Skills perspective) I want to make sure we get the naming right.

I haven't done anything in this Skill before, does it even use that existing query method that you were having trouble with? I had a quick scan but couldn't see it being called from anywhere. It's unrelated to this PR but if it's not needed we can get rid of it.

@hughwilliams94
Copy link
Author

hughwilliams94 commented Oct 10, 2020

Sorry for a slow response, I've updated the message name adding 'skill'.

On the 'unspoken', I had used this to indicate that mycroft does not speak the response returned by Wolfram, but just sends it as a message.

I also couldn't see any usage of the query method in the skill.

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

Successfully merging this pull request may close these issues.

3 participants