Refactor echo.go -> customskill package #28
Conversation
customskill/request/request_test.go
Outdated
| } | ||
|
|
||
| /* | ||
| func TestEnvelope_ApplicationIDValid(t *testing.T) { |
There was a problem hiding this comment.
This can be removed. It's a hold over from some old test code.
| ) | ||
|
|
||
| var ( | ||
| s customskill.Skill |
There was a problem hiding this comment.
This could be a one-line var s customskill.Skill
There was a problem hiding this comment.
do one letter variable names pass golint? I know in python its an issue I'm not sure for Go though.
There was a problem hiding this comment.
I have not been running golint. I will work on getting that setup
There was a problem hiding this comment.
got golint up and running and it didn't seem to complain about this.
customskill/skill.go
Outdated
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| type Skill struct { |
There was a problem hiding this comment.
This should be moved to types.go
| @@ -7,6 +7,7 @@ import ( | |||
| ) | |||
|
|
|||
| // Request Functions | |||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | |||
| func (this *EchoRequest) VerifyTimestamp() bool { | |||
There was a problem hiding this comment.
I haven't yet implemented this in customskill so perhaps the deprecation notice shouldn't exist here. I would be tempted to add this as an optional property of Skill which specified the maximum age a request would be valid for. Thoughts?
| @@ -24,18 +26,22 @@ func (this *EchoRequest) VerifyAppID(myAppID string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | |||
| func (this *EchoRequest) GetSessionID() string { | |||
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
| func (this *EchoRequest) GetSessionID() string { | ||
| return this.Session.SessionID | ||
| } | ||
|
|
||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | ||
| func (this *EchoRequest) GetUserID() string { |
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
| func (this *EchoRequest) GetUserID() string { | ||
| return this.Session.User.UserID | ||
| } | ||
|
|
||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | ||
| func (this *EchoRequest) GetRequestType() string { |
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
| func (this *EchoRequest) GetRequestType() string { | ||
| return this.Request.Type | ||
| } | ||
|
|
||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | ||
| func (this *EchoRequest) GetIntentName() string { |
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
| @@ -44,6 +50,7 @@ func (this *EchoRequest) GetIntentName() string { | |||
| return this.GetRequestType() | |||
| } | |||
|
|
|||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | |||
| func (this *EchoRequest) GetSlotValue(slotName string) (string, error) { | |||
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
| @@ -52,11 +59,13 @@ func (this *EchoRequest) GetSlotValue(slotName string) (string, error) { | |||
| return "", errors.New("Slot name not found.") | |||
| } | |||
|
|
|||
| // Deprecated: Please use the github.com/mikeflynn/go-alexa/customskill package | |||
| func (this *EchoRequest) AllSlots() map[string]EchoSlot { | |||
There was a problem hiding this comment.
I didn't add this to customskill. I'm not convinced we need these little helper functions.
|
Friendly ping @mikeflynn to give this a look over. FYI: I've developed a test skill using this and it works well. |
customskill/response/response.go
Outdated
| return e | ||
| } | ||
|
|
||
| func (e *Envelope) SetStandardCard(title string, content string, smallImg string, largeImg string) *Envelope { |
There was a problem hiding this comment.
for functions with the same parameter type you can just specify the type at the end of the list something like:
SetStandardCard(title, content, smallImg, largImg string) *Envelope
There was a problem hiding this comment.
good catch. I will go back and consolidate instances of this
customskill/response/types.go
Outdated
| Reprompt *Reprompt `json:"reprompt,omitempty"` | ||
| Card *Card `json:"card,omitempty"` | ||
| Directives []interface{} `json:"directives,omitempty"` | ||
| ShouldEndSession bool `json:"shouldEndSession"` |
There was a problem hiding this comment.
so I was doing some testing today with the AudioPlayer and VideoPlayer and I think there are some cases where shouldEndSession actually needs to be omitted. I was getting an error saying "shouldEndSession flag is not allowed with LaunchVideoApp.LaunchDirective". I"m not sure what the best way to omit this would be. I'm not sure how to do omit empty with a bool type. Maybe it'll require some kind of wrapper type
There was a problem hiding this comment.
We could omit it using a boolean pointer and omitempty in the JSON tag. I will update
customskill/skill.go
Outdated
| } | ||
|
|
||
| switch reflect.TypeOf(e) { | ||
| case reflect.TypeOf(&request.LaunchRequest{}): |
There was a problem hiding this comment.
would it be possible to do something like the type switch from here:
https://github.com/golang/go/wiki/Switch#type-switch
maybe that would eliminate the need for the reflect package?
EDIT: I think it would also remove the need for the explicit type assertion later in the case as well.
There was a problem hiding this comment.
not sure. Let me look into it and get back to you
customskill/skill.go
Outdated
| ) | ||
|
|
||
| type Skill struct { | ||
| ValidApplicationIDs []string |
There was a problem hiding this comment.
I like the array of IDs idea.
customskill/skill.go
Outdated
| ValidApplicationIDs []string | ||
| OnLaunch func(*request.LaunchRequest) (*response.Envelope, error) | ||
| OnIntent func(*request.IntentRequest, *request.Session) (*response.Envelope, error) | ||
| OnSessionEnded func(endedRequest *request.SessionEndedRequest) error |
There was a problem hiding this comment.
did you mean to have the endedRequest variable name here? or just the parameter type?
There was a problem hiding this comment.
Maybe? I'll take a look
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| var jsonUnmarshal = json.Unmarshal // Used to enable unit testing |
There was a problem hiding this comment.
how does this work exactly? I've never seen this done before.
There was a problem hiding this comment.
By using a variable for the json.Unmarshal function (instead of the function itself) we can mock it out for unit testing.
|
I like the idea of add VideoPlayer support but I don't want to make this PR any larger than it already is. |
|
Let me know when the discussed changes are on the branch and I'll do a final review. |
|
Happy to make the changes but I first wanted to make sure everyone is ok with the |
|
I generally like to keep things a simple as possible for the user and let the library shoulder the complexity, so I'd vote for not exposing it to the user/developer. Since it's just a wrapper for the request/response I don't think there's any benefit to accessing it. |
|
Hey gang, finally had time to address this PR (new laptop + work peak = no time :( ). All of @rking788's review comments have been addressed. I also implemented the Envelope refactor that @mikeflynn supportted in his last comment. All files in |
|
This seems like it will do what it's intended to do (rewrite of echo.go to a subproject) but I'm starting to worry that we're losing track of the goal, which is to make writing an Alexa skill as easy as possible. 100% we need to break up "skillserver" so that people can integrate it in to their existing APIs and use the features that they want, but we can't break it up so much that making an Alexa app is back to being a pain in the ass. Again, this specific PR looks fine. I haven't made an app with it but the diff looks great...however, before we talk merging to the master, we need to go back to the examples and make sure the core goals are met. We need the following examples:
Any other use cases I'm missing? I think those are core set. If we have a good story to tell on those three then we've got something that is a true "2.0"...but not really a 2.0 because golang isn't down with versions. Thoughts? I admittedly hijacked this PR with this but I've had some time at the end of a crazy busy year to think about this project more and I wanted to get this out there. (Unless I'm missing something, the example in this branch doesn't actually do anything regarding handling an Alexa Skill request so we need do something there regardless.) |
|
Thanks for taking the time to provide this feedback. I agree with you 100% that this PR is not quite "master-worthy" and that more examples are needed. My ultimate plan was to provide examples for different types of apps (normal custom skill, skill with audio player, skill with Echo Show display features, etc) but the examples you mention are definitely higher-priority and ones I had not considered. I will work on getting those written next. As for core use cases I'd like to propose one more: the ability to use this package to write a custom skill, in Go, for AWS Lambda. While there isn't yet official support for Go with Lambda (though it has been announced that it is coming) there are various shims to support this use case. I'm happy to provide an example of this. So, moving forward how about we merge this PR into the refactor branch now and my next PR will be examples to support the examples you mentioned (thoughts on where to put them?) and my Lambda example. I'll do them as 4 separate short PRs unless you prefer one big PR. |
|
Yes, I like the Lambda use case. There are a few project out there that already make deploying go to lambda pretty easy so we can target those for now. Agreed on merging this in, I just wanted to put hijack this PR momentarily to refocus. |
This is a refactor of the code inside of
echo.goto a new subpackage calledcustomskill.This refactor introduces a new concept, called an
envelope, to both therequestandresponse. This envelope contains both the metadata for the request or response and then the request or response itself. Ideally the envelope would never be exposed to the developer and would be used, behind the scenes, to properly unmarshal and marshal the request and response respectively. It also allows the use of familiar names for the request inside of the envelope (requestversus the made-upbodyused currently) Currently this is mostly achieved except with response where the envelope is exposed. I have a proposal to address this below (see Proposal)Request
Request unmarshaling is performed in
customskill/request/request.goand is achieved using theBootstrapFromJSONfunction. This function uses a temporary "dummy" envelope to determine the request type. Once the request type is determined the correct type of request envelope is used to properly unmarshal the request. Instead of returning the request envelope at this stage the envelope's metadata and request is returned. This is done to mirror the way other Alexa SDKs work and to not expose the concept of the envelope to the user.Tests for
customskill/request/request.goare incustomskill/request/request_test.goand currently stand at 100% coverage.Response
Request logic is present in
customskill/response/response.goand differs from the code inecho.goslightly. Firstly, most functions are now prefixed withSetto properly communicate what they are used for. Secondly, theString()function has been updated to properly implement theStringerinterface (returning just a string).Tests for
customskill/response/response.goare incustomskill/response/response_test.goand currently stand at 100% coverage.Custom Skill
Custom skill handling logic is present in
customskill/skill.go. The newSkillstruct defines a customskill's valid application IDs and request handlers. TheHandlerfunction is passed anio.Writerand the JSON envelope and returns just an error. It bootstraps request, validates the application ID, and calls the appropriate handler for a response. Finally it marshals the response and writes it to theio.Writer.Tests for
customskill/skill.goare incustomskill/skill_test.goand currently stand at 100% coverage.Proposal/Question
It's arguable if the concept of "envelopes" should exist at all in the
requestandresponsepackages. One could argue they are actually a concept of the custom skill logic and should exist in the rootcustomskillpackage. The package could be refactored to move enevelope-related logic to the rootcustomskillpackage. If done therequestandresponsepackages would be "more pure" but it would also turn therequestpackage into a package of nothing by types. Thoughts? If we did do thisresponse.gowould no longer expose the concept of the envelope.