bespoken / virtual-alexa

:robot: Easily test and debug Alexa skills programmatically
https://bespoken.io
Apache License 2.0
112 stars 35 forks source link

Added a union type for a more self-documenting type than IResponse wh… #96

Closed dgreene1 closed 5 years ago

dgreene1 commented 5 years ago

Completed the following:

codecov[bot] commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@3904865). Click here to learn what that means. The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #96   +/-   ##
=========================================
  Coverage          ?   95.41%           
=========================================
  Files             ?       28           
  Lines             ?     1308           
  Branches          ?      187           
=========================================
  Hits              ?     1248           
  Misses            ?       60           
  Partials          ?        0
Impacted Files Coverage Δ
src/Index.ts 100% <100%> (ø)
src/impl/SkillInteractor.ts 100% <100%> (ø)
src/core/VirtualAlexa.ts 95.78% <100%> (ø)
src/core/SkillResponse.ts 91.04% <75%> (ø)
src/dialog/DelegatedDialogResponse.ts 81.81% <77.77%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3904865...225e64e. Read the comment docs.

jkelvie commented 5 years ago

Hi @dgreene1 thank you for this PR. Another approach to this issue - make DialogResponse a sub-type of SkillResponse. Though in some ways this is less elegant (as there are many methods on SkillResponse not relevant to a dialog response), this is simpler for consumers to work with, as the vast majority of responses are SkillResponses. After giving some consideration to this issue, I believe that is the most humane approach. What do you think?

dgreene1 commented 5 years ago

I don’t have a strong preference, but I think you are on the money with prefering a solution that makes the lives of the consumer easier (which does not surprise me since your libraries are fantastic!).

But to clarify, what type would utter(“”) return in your option?

jkelvie commented 5 years ago

The response would always be SkillResponse. There would be a responseType property, as you have added.

For the subclass DelegatedDialogResponse, the prompt() method would return the prompt property value on delegatedDialogResponse. Other methods would throw an exception indicating that the method is not supported as it is a DialogResponse, and not coming from the skill.

I believe this is the most humane approach for implementers. It also allows for calling one consistent method for getting the speech output across the types (prompt())

dgreene1 commented 5 years ago

The response would always be SkillResponse. There would be a responseType property, as you have added. For the subclass DelegatedDialogResponse, the prompt() method would return the prompt property value on delegatedDialogResponse. Other methods would throw an exception indicating that the method is not supported as it is a DialogResponse, and not coming from the skill.

sounds good @jkelvie :)

dgreene1 commented 5 years ago

I agree that your proposed solution would work best. I look forward to seeing your team's product. I will close this issue in the meantime.