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

Jump to bottom

Add get_response method as a synchronous alternative to converse #1278

Merged
merged 5 commits into from Dec 7, 2017
Merged

Add get_response method as a synchronous alternative to converse #1278

merged 5 commits into from Dec 7, 2017

Conversation

MatthewScholefield
Copy link
Contributor

@MatthewScholefield MatthewScholefield commented Dec 4, 2017 •
edited

Documentation:

def get_response(self, dialog, data=None):
    """
    Ask and return a response from the user for the given dialog

    Args:
        dialog (str): Dialog file to read to the user
        data (str): data used to render the dialog
    Returns:
        any: None or transcription user's reply
    """
    pass

Example usage:

color = self.get_response('what.is.your.favorite.color')
if color:
    self.speak('My favorite color is also ' + color)

@MatthewScholefield MatthewScholefield added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. labels Dec 4, 2017
@pep8speaks
Copy link

pep8speaks commented Dec 5, 2017 •
edited

Hello @MatthewScholefield! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 07, 2017 at 21:50 Hours UTC

@MatthewScholefield MatthewScholefield force-pushed the feature/get_response branch 2 times, most recently from d345e87 to 98d8506 Compare December 5, 2017 01:25
LearnedVector
LearnedVector approved these changes Dec 5, 2017
View changes
Copy link
Contributor

@LearnedVector LearnedVector left a comment

Choose a reason for hiding this comment

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

LGTM! once ake approves we are good to merge

@forslund forslund added the Status: Work in progress PR being actively worked on, not yet ready for review. label Dec 5, 2017
forslund
forslund reviewed Dec 6, 2017
View changes
mycroft/client/speech/main.py Outdated
@@ -56,6 +56,10 @@ def handle_utterance(event):
ws.emit(Message('recognizer_loop:utterance', event))


def handle_unknown():
ws.emit(Message('speech.recognition.unknown'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@penrods suggested using recognizer_loop:x internally in the speech client and using mycroft.x.y on the interprocess message bus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this has been changed to recognizer_loop:speech.recognition.unknown and mycroft.speech.recognition.unknown. Let me know if any other name sounds better.

@MycroftAI MycroftAI deleted a comment Dec 6, 2017
@MycroftAI MycroftAI deleted a comment Dec 6, 2017
@MycroftAI MycroftAI deleted a comment Dec 6, 2017
@MycroftAI MycroftAI deleted a comment Dec 7, 2017
penrods
penrods suggested changes Dec 7, 2017
View changes
mycroft/client/speech/listener.py Outdated
@@ -152,6 +152,7 @@ def transcribe(self, audio):
text = "pair my device" # phrase to start the pairing process
LOG.warning("Access Denied at mycroft.ai")
except Exception as e:
self.emitter.emit('recognizer_loop:peech.recognition.unknown')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "peech" intentional?

mycroft/client/speech/main.py Outdated
@@ -56,6 +56,10 @@ def handle_utterance(event):
ws.emit(Message('recognizer_loop:utterance', event))


def handle_unknown():
ws.emit(Message('mycroft.mycspeech.recognition.unknown'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "mycspeech" intentional? Looks like a search/replace error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, no idea how I failed at typing that much. Fixed now.

@penrods penrods self-requested a review December 7, 2017 18:09
Steve Penrod added 3 commits December 7, 2017 15:58
Tweak get_response() doc and behavior
db4bb5f 
Several minor documentation changes, plus:
* 'cancel' now has to be an exact match
* Cancel events return None instead of the spoken cancelation string
* Reduced timeout to 10 seconds instead of 20
* Changed 'text' to 'announcement' and simplified logic
Remove unused 'isfile' import
2e9622d
Extend timeout, randomize repeat announcement
82c3a19 
Extended the timeout to longer than the listener default (in case of
operating in a noisy environment).
penrods
penrods approved these changes Dec 7, 2017
View changes
Copy link
Contributor

@penrods penrods left a comment

Choose a reason for hiding this comment

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

I'm good with this after the changes we have made.

MatthewScholefield
MatthewScholefield commented Dec 7, 2017
View changes
mycroft/skills/core.py Outdated
def is_cancel(utterance):
return utterance in cancel_words

def on_valid_default(utterance):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds a bit funny since the original parameter isn't called on_valid.

MatthewScholefield
MatthewScholefield commented Dec 7, 2017
View changes
Copy link
Contributor Author

@MatthewScholefield MatthewScholefield left a comment

Choose a reason for hiding this comment

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

LGTM, merging.

@MatthewScholefield MatthewScholefield merged commit 50a5b21 into dev Dec 7, 2017
@MatthewScholefield MatthewScholefield deleted the feature/get_response branch December 7, 2017 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@forslund forslund

@LearnedVector LearnedVector

@penrods penrods

Assignees
No one assigned
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: Work in progress PR being actively worked on, not yet ready for review. Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MatthewScholefield @pep8speaks @forslund @LearnedVector @penrods