Handle multiple intents with the same name

[forslund]

Description

This implements Alternative 1 from #2920

• It will raise a ValueError if two named IntentBuilders with the same name is registered
• If anonymous IntentBuilders are used they will be automatically given an unique name

The handling for enable-disable intent was updated to make sure that the intent was removed from the list of registered_intents, for re-enabling it's moved to a list of detached_intents

How to test

Add a second @intent_handler() decorator to a method ensure both can be used to trigger the handler and that the handler is triggered exactly once.

Contributor license agreement signed?

CLA [ Yes ]

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results). Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[NeonDaniel]

If an active intent is re-registered, it raises an exception that wasn't previously there which results in a spoken error. I think this could be safely caught in the mycroft_skill class and simply log some warning.

2021-10-18 13:33:18.706 | ERROR    | 2982927 | mycroft.skills.mycroft_skill.mycroft_skill:on_error:914 | An error occurred while processing a request in Wikipedia Skill
Traceback (most recent call last):
  File "/home/d_mcknight/PycharmProjects/mycroft-core/mycroft/skills/mycroft_skill/event_container.py", line 73, in wrapper
    handler(message)
  File "/opt/mycroft/skills/wiki.neon/__init__.py", line 79, in handle_intent
    self.enable_intent("WikiMore")
  File "/home/d_mcknight/PycharmProjects/mycroft-core/mycroft/skills/mycroft_skill/mycroft_skill.py", line 1106, in enable_intent
    self.register_intent(intent, None)
  File "/home/d_mcknight/PycharmProjects/mycroft-core/mycroft/skills/mycroft_skill/mycroft_skill.py", line 993, in register_intent
    return self._register_adapt_intent(intent_parser, handler)
  File "/home/d_mcknight/PycharmProjects/mycroft-core/mycroft/skills/mycroft_skill/mycroft_skill.py", line 969, in _register_adapt_intent
    raise ValueError(f'The intent name {name} is already taken')
ValueError: The intent name WikiMore is already taken

[forslund]

Thanks for flagging this. I think the registering should raise the exception but it should be caught and logged by the enable_intent. Does that sound like a good solution?

[pep8speaks]

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! :beers:

Comment last updated at 2021-10-23 11:28:16 UTC

[NeonDaniel]

Thanks for flagging this. I think the registering should raise the exception but it should be caught and logged by the enable_intent. Does that sound like a good solution?

I think for backwards-compatibility these exceptions should be handled (catch and log), maybe with a deprecation notice that future releases will raise an exception if an active intent is activated.

[forslund]

It's now more than a year since the last concern raised here was pushed (I think), is this something the core team is interested in or should I close the PR?

[krisgesling]

Hey Ake, I think it's a good change - seems like everything is accounted for and it passed all the tests. So if there's no opposition I think we can merge it in.

I'll do this on Monday/Tuesday unless I hear otherwise.