Handle multiple intents with the same name (#2921) #235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one. * Add tests for intent collisions * Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. * Add move logic to find if intent is detached MycroftSkill.enable_intent() will now check if the intent is detached before trying to re-enable it. * Lock updates of intents This should avoid some race conditions that may occur if multiple threads tries to enable / disable intents
JarbasAl
added
bug
Codecov Report
@@ Coverage Diff @@
## dev #235 +/- ##
==========================================
+ Coverage 50.35% 54.90% +4.55%
==========================================
Files 119 156 +37
Lines 10077 9350 -727
==========================================
+ Hits 5074 5134 +60
+ Misses 5003 4216 -787
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
JarbasAl
force-pushed
the
port/mycroft_fix
branch
from
969ed81 to
6302211
Compare
JarbasAl
force-pushed
the
port/mycroft_fix
branch
from
789c711 to
1f8f074
Compare
NeonDaniel
approved these changes
Nov 28, 2022
| @@ -21,7 +21,7 @@ | |||
| from inspect import signature | |||
| from itertools import chain | |||
| from os.path import join, abspath, dirname, basename, exists, isfile | |||
| from threading import Event | |||
| from threading import Event, Lock | |||
There was a problem hiding this comment.
I've started using RLock for applications like these in case different wrappers are added/refactored; so long as only one thread is mutating things at a time, we should be okay
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #233