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

fix/intent_api #2786

Merged
merged 5 commits into from
Dec 31, 2020
Merged

fix/intent_api #2786

merged 5 commits into from
Dec 31, 2020

Conversation

JarbasAl
Copy link
Contributor

the latest intent refactor #2599 seems to have forgotten the intent api #2468 was a thing and borked it

this PR fixes it and does some extra cleanup moving things around to more logical places. The bus handlers were split across Padatious and IntentService and have now been unified

To test see #2468

This one can't be monkey patched at runtime, please review and merge

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Dec 29, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

Thanks for fixing my mess @JarbasAI. I thought I did the api correct but seem to have messed up. Can you check out the failing unittests? (I did create some to cover the intent api)

@forslund
Copy link
Collaborator

forslund commented Dec 30, 2020

Should we make sure to prepend an _ to the bus handlers so we don't have to worry about breaking the api if we move those around in the future? Or maybe add that as a 21.02 task?

Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

In addition to the change requests the test/unittests/skills/test_intent_service.py L290 needs to be changed to self.intent_service.handle_adapt_manifest(msg)

With those changes all the tests pass again.

mycroft/skills/intent_service.py Show resolved Hide resolved
mycroft/skills/intent_service.py Outdated Show resolved Hide resolved
@krisgesling krisgesling added Status: Change requested Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. labels Dec 30, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

forslund
forslund previously approved these changes Dec 30, 2020
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Nice, apart from a couple of spelling mistakes it looks good. Approved

I think the changes in the object api are ok since these methods works through the messagebus and is of little use when called directly.

mycroft/skills/intent_service.py Outdated Show resolved Hide resolved
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling
Copy link
Contributor

Thanks to you both!!

@krisgesling krisgesling merged commit f1d7141 into MycroftAI:dev Dec 31, 2020
@JarbasAl JarbasAl deleted the fix/intent_api branch December 31, 2020 03:35
@JarbasAl JarbasAl mentioned this pull request Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants