alexa-js / alexa-utterances

generate expanded utterances for Amazon Alexa from a template string
MIT License
152 stars 28 forks source link

switch from curly braces to parens for groupers #2

Closed rickwargo closed 8 years ago

rickwargo commented 9 years ago

To support custom slot types for alexa-app, I had to switch your convention of using curly braces to wrap groups to parens. The curly brace construct is used to represent a custom slot type. I know this will affect all of your code, but I think going forward it makes it easier to embrace the custom slot types.

An example of a schema that makes use of this new construct follows; note the utterances:

{
    "slots": {
        "Level": "LIST_OF_LEVELS"
    },
    "slot_types": [
        {
            "name":"Level",
            "values":["beginner","easy","normal","medium","average","standard","advanced","challenging","hard","difficult"]
        }
    ],
    "utterances": [
        "(change|switch) (level to|to level|to) {Level} (|problems|questions|challenges)",
        "new level {Level}"
    ]
}
mreinstein commented 9 years ago

@rickwargo thanks for the PR!

To support custom slot types

Can you explain what custom slot types are? Is this a new feature in amazon alexa, or something else?

rickwargo commented 9 years ago

Custom Slot Types are a new feature in the Amazon Alexa API.

Amazon posted a migration page to explain

Looks like Amazon is phasing out LITERAL in favor of custom slot types (started early October). So, instead of how everyone has done it, it creates a custom slot and lists the potential values for that. You can then use that slot name as a "variable" in the utterances (see my example above). Amazon says it cuts down on the length and complexity.

Unfortunately, this completely changes how you have done things in the past and would require a code change. Because of LITERALs being deprecated, I did not attempt to support that particular usage.

simplydallas commented 8 years ago

I do not recommend integrating this pull request. Instead of changing from curly braces, I added a simple convention for empty curly brace outputs. {-|NAME} outputs as {NAME} instead of what would originally output as NAME. This is a simple find and replace for {-| with {. This requires very little code change, is easy to implement in existing projects, and still allows for the new amazon built in and custom types. I can make a pull request if wanted but it is just adding .replace(/\{\-\|/g, "{") to the string output.

example code snippet:

alexaApp.intent("TokenNumber",
  {
    "slots":{"AMOUNT":"AMAZON.NUMBER"}
    ,"utterances": [
      "{-|AMOUNT} {token|tokens|}",
    ]
  },

the output for this would be the following three lines:

TokenNumber {AMOUNT} token
TokenNumber {AMOUNT} tokens
TokenNumber {AMOUNT}

This convention allows easily mixing the old lists and the new custom types and built in types in the same utterance file or even in the same intent as well, so it is fully backwards and forwards compatible.

joshskeen commented 8 years ago

@mreinstein What did we decide here? Supporting custom slot types is a pressing need with skills development at the moment.

I think the regex @simplydallas is suggesting seems to work well if we're worried about being backwards compatible. What do you think?

rickwargo commented 8 years ago

I think there is value in thinking of the curly braces as having a single responsibility - to delineate the slot name. Grouping and alternation have been represented in regex's as parens and pipes and I think that metaphor works well here. I know there would be existing code change required to make this work - we could have the curly braces have dual purpose and also be used for grouping for backward compatibility.

mreinstein commented 8 years ago

It's been a while since I've looked at this. Here was where I last left off https://github.com/mreinstein/alexa-utterances/issues/7

if I recall I was leaning torwards adopting Rick's PR, throwing in some other breaking changes, and bumping the version to 2.x.

Work has me slammed at the moment but I'm hoping to have time tomorrow to revisit this.

That said, this is not set in stone. Open for additional comments/ideas.

mreinstein commented 8 years ago

I've found some time today to work on this. Besides backwards compatibility does anyone have objections to using parentheses ( ) ? If so, what are the reasons?

simplydallas commented 8 years ago

I wouldn't underestimate backwards compatibility when it is easy to achieve. Another consideration is that parentheses are already semantically meaningful in JS so mixing that meaning with something else by using it in templating may be awkward to new users. Everyone under the sun uses { and } for templating so it is familiar and fairly friendly to many people already. Simple seems better in this case but this is all easy to tweak so either way will work out fine in the long run.

matt-kruse commented 8 years ago

I would consider backwards compatibility to be very important. As the modules move forward, skill authors won't want to go back and re-work their skills. That's my opinion. :)

joshskeen commented 8 years ago

I think a way forward might be to:

  1. offer a final release with the old syntax, which adds the regex to support custom slot types.
  2. a next version that then removes the {} syntax and replaces it with ()'s. Agree that replacing {'s with ('s makes more sense because the { syntax is overloaded.

Then, users can choose. If they want to update to the latest, they can upgrade to the newest version of the library and update their syntax. We can make sure to note in the README that this will be required. Alexa-app docs can change at that point also.

If they want to stick with the old syntax, they'll still have support for custom slots via the {-|CustomSlotName} + regex by staying with the older version.

mreinstein commented 8 years ago

ok so let me repeat this back to see if we're all on the same page:

1 merge the PR from @simplydallas for the {-|CustomSlotName} syntax

2 publish this to npm with semver minor bump (0.2.0)

3 switch the format to paren (i.e., accept PR from @rickwargo)

4 include other breaking changes as per #7

5 publish 1.0

does that make everyone happy?

joshskeen commented 8 years ago

Yes, I think this solves the problem nicely. Alexa-app can upgrade right away to use the minor version and support the old style utterance syntax with custom slots. Then, with version 3.0 of alexa-app, we can release with the newest alexa-utterances release and the other breaking changes alexa-app has planned. 👍

rickwargo commented 8 years ago

That works for me @mreinstein & @mutexkid. Thanks!

mreinstein commented 8 years ago

@simplydallas assuming this works for you, I can accept a PR from you for the custom slot syntax, or I can merge https://github.com/mreinstein/alexa-utterances/pull/5

what is preferable?

simplydallas commented 8 years ago

5 looks fine to me @mreinstein. @MaxwellPayne was more thorough than I was and added a test for it so we may as well use his cleaner code. ;)

mreinstein commented 8 years ago

Just published 0.2.0 to npm

mreinstein commented 8 years ago

also set up a 1.0 branch:

https://github.com/mreinstein/alexa-utterances/tree/1.0

@rickwargo would you mind re-submitting your PR here?

mreinstein commented 8 years ago

done. I'm not going to push v1.0 to npm, as I don't see a big need for it at the moment. at some point Matt might release a major new version of alexa-app with breaking changes, and then it'd be a good time to coordinate alexa-utterances@1.0

thanks for everyone's feedback! Constructive feedback still welcome, but marking this ticket as resolved.