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
Upgrade Dialogflow to work with V2 API #25975
Conversation
8269585
to
d861c9d
Compare
d861c9d
to
b53dc8f
Compare
This PR is ready for review. |
Is this a breaking change or is this backwards compatible ? |
@@ -84,23 +87,45 @@ class DialogFlowError(HomeAssistantError): | |||
|
|||
def dialogflow_error_response(message, error): | |||
"""Return a response saying the error message.""" | |||
dialogflow_response = DialogflowResponse(message["result"]["parameters"]) | |||
_api_version = get_api_version(message) | |||
if _api_version is V1: |
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.
Use ==
for comparison, not is
. is
is for pointer equivalency.
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 intended pointer equivalency here since it's comparing integers, but I'm happy to change it to ==.
parameters = message["result"]["parameters"] | ||
elif _api_version is V2: | ||
parameters = message["queryResult"]["parameters"] | ||
dialogflow_response = DialogflowResponse(parameters, get_api_version(message)) |
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.
You should be able to use _api_version
here ?
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.
Good eye! Should be fixed now.
@@ -84,23 +87,45 @@ class DialogFlowError(HomeAssistantError): | |||
|
|||
def dialogflow_error_response(message, error): | |||
"""Return a response saying the error message.""" | |||
dialogflow_response = DialogflowResponse(message["result"]["parameters"]) | |||
_api_version = get_api_version(message) |
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.
Please don't prefix parameters with _
if you're using them.
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.
Agreed - that was bad styling on my part. It's fixed now.
req = message.get("result") | ||
action_incomplete = req["actionIncomplete"] | ||
_api_version = get_api_version(message) | ||
if _api_version is V1: |
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.
idem.
) | ||
req = message.get("result") | ||
action_incomplete = req.get("actionIncomplete", True) | ||
if action_incomplete is True: |
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.
if action_incomplete is True: | |
if action_incomplete: |
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.
This looks great ! After reading the code it is backwards compatible 👏
A few minor comments but after that ok to merge 🐬
@balloob I added the changes you requested. Let me know if you want it rebased into one commit. |
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.
Looks great, awesome work ! 🎉
Description:
On October 23, 2019, the V1 API for Dialogflow will be shut down forcing users to use the V2 API. Additionally, new Dialogflow accounts are being forced to use the new API. Currently, home assistant does not support this API, so all old Dialogflow intents will stop working and new Dialogflow accounts cannot create workable intents.
This is my first PR to home-assistant, but I tried to follow the docs closely. I'm sure I have messed something up, so please let me know what I can do to fix it.
Related issue (if applicable):
Fixes #18411 and fixes #14343
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10149
Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
..coveragerc
.If the code does not interact with devices: