-
Notifications
You must be signed in to change notification settings - Fork 60
Refactor cleanup tests #38
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
base: package-refactor-dev
Are you sure you want to change the base?
Refactor cleanup tests #38
Conversation
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 good to me.
@@ -36,10 +36,9 @@ func main() { | |||
http.ListenAndServe(":8080", nil) | |||
} | |||
|
|||
func onLaunch(launchRequest *request.LaunchRequest) (*response.Response, map[string]interface{}, error) { | |||
func onLaunch(launchRequest *request.LaunchRequest, metadata *request.Metadata) (*response.Response, map[string]interface{}, error) { | |||
resp := response.New() | |||
resp.SetEndSession(response.Bool(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.
Should the session stay open if the user is launching the skill? seems like the example could be misleading.
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.
Would it be worth adding some speech here so if the user tries it out then they can at least get some output speech?
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.
Seems reasonably. It's on my "todo" list but I'm currently getting slammed at work. Hope to have time later this week to work on this.
Note: This code change assumes the
Add metadata to customskill handlers
commit is going to be pulled in from #36. Sorry for the noise from that commit.A few little changes here:
Errorf
you can just uset.Run(name, ...)
. I've applied this to all tests.defer
so that has been added to all tests that use mocked functions.got/wanted
overgot/expected
(or similar). Mostly just for standardization.