Skip to content

Conversation

@NeonJarbas
Copy link

@NeonJarbas NeonJarbas commented Nov 18, 2021

make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the skill itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a single class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if

  • skill does not have converse capabilities
  • skill was unloaded/disconnected
  • skill does not want to converse at this time

@NeonJarbas NeonJarbas force-pushed the refactor/converse branch 2 times, most recently from 0cc12d7 to b0534f2 Compare November 18, 2021 17:32
@JarbasAl JarbasAl added the enhancement New feature or request label Nov 18, 2021
@NeonJarbas NeonJarbas force-pushed the refactor/converse branch 2 times, most recently from 184068a to 620f0bb Compare November 18, 2021 18:12
JarbasAl added a commit that referenced this pull request Nov 26, 2021
make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the class itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a dedicated class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if
- skill does not have converse capabilities
- skill was unloaded/disconnected
- skill does not want to converse at this time
JarbasAl added a commit that referenced this pull request Jan 24, 2022
make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the class itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a dedicated class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if
- skill does not have converse capabilities
- skill was unloaded/disconnected
- skill does not want to converse at this time
JarbasAl added a commit that referenced this pull request Feb 1, 2022
make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the class itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a dedicated class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if
- skill does not have converse capabilities
- skill was unloaded/disconnected
- skill does not want to converse at this time
JarbasAl added a commit that referenced this pull request Feb 1, 2022
make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the class itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a dedicated class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if
- skill does not have converse capabilities
- skill was unloaded/disconnected
- skill does not want to converse at this time
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

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

Few comments for consideration

@JarbasAl JarbasAl requested a review from NeonDaniel February 2, 2022 15:11
make converse event based, it was a little odd because it was the only code from MycroftSkill called outside of the class itself

The event based approach also adds support for external skills, eg, Hivemind and OVOSAbstractApp

refactored converse process into a dedicated class like adapt/padatious/fallbacks

added a new converse acknowledge bus message, now converse is only called for skills where converse is implemented.

skills are removed from active skill list if
- skill does not have converse capabilities
- skill was unloaded/disconnected
- skill does not want to converse at this time
@JarbasAl JarbasAl added the refactor code refactor without functional changes label Feb 2, 2022
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

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

Untested but changes look good

@JarbasAl JarbasAl merged commit 4fff98e into OpenVoiceOS:dev Feb 2, 2022
@NeonJarbas NeonJarbas mentioned this pull request Feb 16, 2022
@NeonJarbas NeonJarbas deleted the refactor/converse branch June 8, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor code refactor without functional changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants