Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Bugfix for padatious and converse responses #2476

Merged
merged 4 commits into from
Feb 24, 2020
Merged

Bugfix for padatious and converse responses #2476

merged 4 commits into from
Feb 24, 2020

Conversation

@forslund
Copy link
Collaborator

@forslund forslund commented Feb 20, 2020 •
edited
Loading

Description

  • Forwards messages messages from the padatious service to keep the destination intact.
  • copies context in reply to make sure original message context stays the same.
  • Fix error when the ident is missing during converse handling

How to test

Make sure the hello world skill's "how are you" intent, qa skill and "what's the weather like" intent returns audio.

Contributor license agreement signed?

CLA [ Yes ]

@pep8speaks
Copy link

pep8speaks commented Feb 20, 2020 •
edited
Loading

Hello @forslund! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-22 08:36:01 UTC

@forslund forslund requested a review from JarbasAl February 20, 2020 10:57
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Feb 20, 2020
@forslund forslund force-pushed the bugfix/message-targeting branch 3 times, most recently from 98a4d4b to 8e38398 Compare February 20, 2020 11:12
@forslund forslund changed the title Bugfix for fallbacks and padatious messages Bugfix for fallbacks and padatious and get_response messages Feb 20, 2020
@forslund
Handle missing message idents in converse
1a04322 
Handle the case where context exists but without ident
@forslund
Copy message context on reply
aad961f 
This ensures that the original context isn't modified, for example when
converse is used the reply there would switch the source/destination for
the original message making the skill speak to the wrong destination.
@forslund
Make padatious service forward the intent call
acf3b96
@forslund forslund force-pushed the bugfix/message-targeting branch from 4c9c8ae to acf3b96 Compare February 21, 2020 07:27
@forslund forslund changed the title Bugfix for fallbacks and padatious and get_response messages Bugfix for padatious and converse responses Feb 21, 2020
@forslund forslund force-pushed the bugfix/message-targeting branch from 716c172 to e1fa0e3 Compare February 22, 2020 08:35
@JarbasAl
Copy link
Contributor

JarbasAl commented Feb 23, 2020

this looks good! will do some testing later today, but it seems ready to go

nice work, and great catch with the deepcopy stuff, this could easily trip up someone

@forslund
Copy link
Collaborator Author

forslund commented Feb 24, 2020

Thanks @JarbasAI for all the good feedback. I'm goingto merge this as it is now, if you find anything let me know and I'll do a follow up PR.

@forslund forslund merged commit 493d057 into MycroftAI:dev Feb 24, 2020
@forslund forslund mentioned this pull request Feb 24, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JarbasAl JarbasAl Awaiting requested review from JarbasAl

Assignees

No one assigned

Labels

CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@forslund @pep8speaks @JarbasAl @devs-mycroft