Proposal: separate alexa request/response from http server

[harrisonhjones]

Found this project the other night. Looks great. Any thoughts about separating the http server components and the Alexa request/response into separate packages? I'm looking at developing my Alexa skills using a Lambda Go shim so I don't need the HTTP server part of this package but I like the SSML parts.

Also, I don't see a LICENSE for this package. How are you licensing it?

[mikeflynn]

This has come up a few times before but I haven't had a chance to dig in a refactor it to the point they are separated.

As for license, the skillserver dir originally had a GNU license file, but I just moved it to MIT with the file in the project root so GitHub picks it up.

[harrisonhjones]

I'm happy to attempt the refactor but I'd like to coordinate it ahead of time. Here's my proposal:

• New /customskill directory in root of go-alexa. Imported with github.com/mikeflynn/go-alexa/customskill. Inside of this directory:
  • Move all request and respond related types to a types.go
  • Move echo.go in here and split it up into cards, request, & response go files.
  • Remove mentions of Echo in method calls as they are not needed
  • Tempted to change things like RepromptSSML to SetRepromptSSML as it's clearer
  • (Stretch) Write tests
• New /ssml directory in root of go-alexa. Move SSML builder here
• Refactor skill server to use the above

Thoughts?

[mikeflynn]

This is definitely along the same lines I've been thinking from the start (which is why it's been nested under go-alexa).

The project breakout makes sense and most of these moves should workable without too much logic changing, which is good. I'd hate to change too much on people in one shot as the breaking out of the projects is already a big change.

Anyone else want to weigh on this?

[harrisonhjones]

Proposal for migration:

1. Add the new customskill and ssml directory and COPY code as needed
2. Update skillserver main logic to use these new packages
3. Mark duplicate code in skillserver as deprecated (From GoDoc: "To signal that an identifier should not be used, add a paragraph to its doc comment that begins with "Deprecated:" followed by some information about the deprecation.")
4. Don't contribute any new code to deprecated packages/types

That should be pretty painless for endusers. If they need the new features they can refactor but until they do it will continue to work.

[rking788]

I think all of that sounds reasonable and a useful change for future development.

[mikeflynn]

Sounds good to me. It's only one package now, so the vast majority of people are just using the skillserver as a single piece, meaning they would probably not even know the difference, but since Go isn't big on versions, this is safest way to go.

[harrisonhjones]

Great. I'm happy to start the refactoring but I'm a bit unsure the best way to send in a PR. I prefer small PRs to make them easy to review but I also don't want to commit unfinished code to mainline. How about I fork the repo, develop on a development branch, issue a PR against a development branch on this repo and then, once it's all stable (in a few PRs) issue a PR to bring it into mainline and remove the development branch? Thoughts?

[mikeflynn]

Yup, development branch is definitely the way to go for this.

I might have some free time later this week so let me know if you need any help, but this should be pretty straightforward I think.

[rking788]

FYI i'm also working on dialog support in this PR, #21

i'll be submitting a new commit to add the type safety as discussed. I would imagine this would need to be refactored into possibly a dialog.go file as part of this work after its merged.

[mikeflynn]

Yup, we'll have to have a little air traffic control to get both of these projects landed, but lets just see which is done first and go from there.

[harrisonhjones]

Hey @mikeflynn (and anyone else @rking788?) I'm working on the refactor for the custom skills request / response and I wanted y'all's input. As I have it now I've renamed EchoRequest (the high-level request struct) to Envelope (not married to the name) and built a custom JSON unmarshaller for it. You hand it the incoming question to your skill and you get an Envelope back. This Envelope has the following signature:

type Envelope struct {
	Version string  `json:"version"`
	Session Session `json:"session"`
	Context Context `json:"context"`
	Request interface{} `json:"request"`
}

The part I want feedback on is what I've done with the Request. I wanted to do away with our single Request type that has lots of fields that aren't used for any particular request (all request types smooshed into one) so Request is an interface{} and its populated automatically by the unmarhsaller with the appropriate *Request type. Unfortunately that means that you have to do some fancy work to actually read the data. You need to use reflection to check the type and then cast it to that type. Depending on how you implement your custom skill I don't think this is too much of a big deal. You simple do something like switching off of the type reflect returns and then go from there. Thoughts?

[rking788]

Would it make sense to provider specific getters for the different request types that do the type assertion/casting and returns either that request type or an error if Request is not of that type?

Just seems like that would be convenient for cases where the client usually knows what type of request should be of a specific type but also leaves flexibility for clients to handle the Request property more dynamically if they want.

[harrisonhjones]

@rking788 I don't think there is a way to avoid the casting by the client (if you can figure out a way I'd appreciate a code snippet). What we could do it something like Envelope.GetType which handles the reflection for the client and then all they need to do is perform the cast.

I don't think clients usually do know what the type of an incoming request is ahead of time. How would they (or maybe I am misunderstanding?)