-
Notifications
You must be signed in to change notification settings - Fork 28
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
Test for incorrect cartesian product. #19
base: master
Are you sure you want to change the base?
Conversation
so just to add some historical context around this. ^ I really recommend reading this document carefully ^ The original PR which created |
another thing to keep in mind: there is an improved slot type, which reduces/removes the need for these expansions. These are wonderful for keeping the utterances file to a minimum. Wherever possible, people should be using this, as described here: https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/docs/migrating-to-the-improved-built-in-and-custom-slot-types |
Makes sense @mreinstein. For the purposes of this PR, are you saying the way these are expanded is correct as it stands (if so, feel free to just close this PR)? Then, is the documentation incorrect in that case as it states that a cartesian product is being built? |
I haven't looked at the test cases in this PR very carefully to assert their correctness. I posted the links because the people that originally brought the |
Looking forward to hear from you re: correctness @mreinstein when you have time ;) LMK if I can be of any help. |
@mreinstein Thank you. I read the doc ref'd above and this makes sense. If I understand the documentation correctly we don't always need to generate an exhaustive set of utterances, only a representative set. Thus exhaustive can often safely default to And for the new types we don't need to generate even a large representative set of examples. But given that why then don't we modify |
+1 on supporting well known amazon types @mseminatore, potentially |
@mseminatore @dblock this was the old discussion around removing things from the next version of alexa-utterances that has breaking changes: #7 The discussion I started was around dropping some of the stuff in if you look in the 1.0 branch you'll see the work I did to trim down this module, but haven't merged it. Waiting for a time when Still open to discussion on this |
Following up on alexa-js/alexa-app#115, which turned into #16 here but didn't describe the problem quite fully I believe.
This test demonstrates the issue, but I could be missing something? The documentation states that this should be the full cartesian product of all shortcut values. So should it be identical to a full expansion in this case?