-
-
Notifications
You must be signed in to change notification settings - Fork 38.2k
Move HassIntent handler code into helpers/intent #12181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 24 commits
eedb3d0
bf015c9
b3d7bf6
8651566
c842be2
04b407a
c479075
13d0d9a
ac5d2e5
2dcfe18
861cdc5
2214096
90280d7
f287689
7992a73
5e987e8
5496efd
cb8ab73
2d9becd
0d955d3
1262ba3
ed1bcf2
6cfce83
95d30c6
42273fc
a43bc92
b2ad2f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,27 @@ | ||
| """Module to coordinate user intentions.""" | ||
| import asyncio | ||
| import logging | ||
| import re | ||
|
|
||
| import voluptuous as vol | ||
|
|
||
| from homeassistant.core import callback | ||
| from homeassistant.exceptions import HomeAssistantError | ||
| from homeassistant.helpers import config_validation as cv | ||
| from homeassistant.loader import bind_hass | ||
| from homeassistant.const import ATTR_ENTITY_ID | ||
|
|
||
|
|
||
| DATA_KEY = 'intent' | ||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| INTENT_TURN_OFF = 'HassTurnOff' | ||
| INTENT_TURN_ON = 'HassTurnOn' | ||
| INTENT_TOGGLE = 'HassToggle' | ||
|
|
||
| SLOT_SCHEMA = vol.Schema({ | ||
| }, extra=vol.ALLOW_EXTRA) | ||
|
|
||
| DATA_KEY = 'intent' | ||
|
|
||
| SPEECH_TYPE_PLAIN = 'plain' | ||
| SPEECH_TYPE_SSML = 'ssml' | ||
|
|
||
|
|
@@ -87,7 +94,7 @@ class IntentHandler: | |
| intent_type = None | ||
| slot_schema = None | ||
| _slot_schema = None | ||
| platforms = None | ||
| platforms = [] | ||
|
|
||
| @callback | ||
| def async_can_handle(self, intent_obj): | ||
|
|
@@ -117,6 +124,72 @@ def __repr__(self): | |
| return '<{} - {}>'.format(self.__class__.__name__, self.intent_type) | ||
|
|
||
|
|
||
| def fuzzymatch(name, entities): | ||
| """Fuzzy matching function.""" | ||
| matches = [] | ||
| pattern = '.*?'.join(name) | ||
| regex = re.compile(pattern) | ||
| for entity in entities: | ||
| match = regex.search(entity) | ||
| if match: | ||
| matches.append((len(match.group()), match.start(), entity)) | ||
| return [x for _, _, x in sorted(matches)] | ||
|
|
||
|
|
||
| class ServiceIntentHandler(IntentHandler): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you specify in both the name and doc of this class that it is only working for services that call entities and that it needs a name slot that will be mapped to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. I think having the slot schema in the class also makes it clearer. |
||
| """Service Intent handler registration. | ||
|
|
||
| Service specific intent handler that calls a service by name/entity_id. | ||
| """ | ||
|
|
||
| slot_schema = { | ||
| 'name': cv.string, | ||
| } | ||
|
|
||
| def __init__(self, intent_type, domain, service, speech): | ||
| """Create Service Intent Handler.""" | ||
| self.intent_type = intent_type | ||
| self.domain = domain | ||
| self.service = service | ||
| self.speech = speech | ||
|
|
||
| @asyncio.coroutine | ||
| def async_handle(self, intent_obj): | ||
| """Handle the hass intent.""" | ||
| hass = intent_obj.hass | ||
| slots = self.async_validate_slots(intent_obj.slots) | ||
| response = intent_obj.create_response() | ||
|
|
||
| name = slots['name']['value'] | ||
| entities = {state.entity_id: state.name for state | ||
| in hass.states.async_all()} | ||
| entity_name = name.replace(' ', '_').lower() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment didn't stick, but sine we convert spaces to _ in friendly names this is needed to match and entity names will never contain a space. But this is better done in the matching code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We match against entity name and not id right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, should be both since that code should create a dict of id: name but apparently does not for whatever reason and is just creating a list of entity_ids. Flipped it to match on name. |
||
| entity_name = entity_name.replace('the_', '') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment why this is? Also, it seems very English focused
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, and again with the above this may no longer be necessary but then again it might, but this logic should be moved into that function. But a typical utterance is 'Turn on the living room lights' so we get 'the living room lights' for the entity name. Not that we are trying to match everything but it seemed a common enough use. But I can remove it since there is no need to make it too specific and turn on livign room lights is fine.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds like an issue that should be fixed by things launching intents?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, makes sense that it would be done by extending utterances instead. |
||
|
|
||
| matches = fuzzymatch(entity_name, entities) | ||
| entity_id = next((x for x in matches if self.domain in x), None) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's with this check? Isn't domain for now always
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well my plan is to add things like covers and media players, so an Intent like HassOpenCover declares it's domain we then match against cover.garage_door and not switch.garage_door or sensor.garage_door.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it's:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would we add another passed variable instead of just 'if domain not Homeassistant'? And I don't think it's out of scope since the whole intention of this is to lay some groundwork for adding other intents. Otherwise all of this has just been rearranging things with zero added value beyond cleanup.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you 100% guarantee that you can always derive the filter from the service domain? Because if you can't, an extra variable is necessary. A cleanup so that you can start building on top is perfectly fine to be in a single pr. If you're adding functionality you need to use it and you need to test it. Neither is happening.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The best filter would be to filter by entites that support the given domain.service
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. However, one thing is that Home Assistant core should not be aware of the different components. So if we would want something like this, we would have to make it part of service registration, and then the service registry needs to know about which attribute should map to the entity… it's a bit too much. So I think that passing the right filter at the moment we define the intent is great, that way the logic remains inside the components. (light intent will specify light filter, etc) |
||
| if not entity_id: | ||
| entity_id = matches[0] if matches else None | ||
| _LOGGER.debug("%s matched entity: %s", name, entity_id) | ||
|
|
||
| response = intent_obj.create_response() | ||
| if not entity_id: | ||
| response.async_set_speech( | ||
| "Could not find entity id matching {}".format(name)) | ||
| _LOGGER.error("Could not find entity id matching %s (%s)", name, | ||
| entity_name) | ||
| return response | ||
|
|
||
| yield from hass.services.async_call( | ||
| self.domain, self.service, { | ||
| ATTR_ENTITY_ID: entity_id | ||
| }, blocking=True) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove blocking, it has been causing trouble for Alexa/Google Assistant as turning on can sometimes take a long time and we don't actually know if a service is successful. |
||
|
|
||
| response.async_set_speech( | ||
| self.speech.format(name)) | ||
| return response | ||
|
|
||
|
|
||
| class Intent: | ||
| """Hold the intent.""" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,6 @@ | |
| 'ephem', | ||
| 'evohomeclient', | ||
| 'feedparser', | ||
| 'fuzzywuzzy', | ||
| 'gTTS-token', | ||
| 'ha-ffmpeg', | ||
| 'haversine', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add this back where we import fuzzywuzzy. I remember it spamming a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you've removed fuzzywuzzy, ok too 👍