Update ssml.cleanse to stop removing spaces before decimals #236

Merged
merged 4 commits into from May 9, 2017

Conversation

Projects
None yet
3 participants
Contributor

aminimalanimal commented May 5, 2017 edited

alexa-app was unexpectedly stripping spaces that occur before periods. Since periods are also decimals, this produced issues with the expected output of our bartender app's card text.

Things like:

Ingredients: 2 oz Tanqueray No. TEN Gin, .5 oz Heavy cream, .5 oz Fresh lemon juice, .5 oz Fresh lime juice, .75 oz Simple syrup (one part sugar, one part water), 3 dashes Orange flower water, 1 Fresh egg white, and Club soda

were showing up as:

Ingredients: 2 oz Tanqueray No. TEN Gin,.5 oz Heavy cream,.5 oz Fresh lemon juice,.5 oz Fresh lime juice,.75 oz Simple syrup (one part sugar, one part water), 3 dashes Orange flower water, 1 Fresh egg white, and Club soda

I think that this sort of cleansing should probably not be done at all, instead letting app developers to have control over their output, but this PR only removes the item that affected us. Perhaps the others will always be ideal to strip spaces for, but it's a hard thing to predict—after all, our use-case wasn't immediately obvious. :)

aminimalanimal added some commits May 5, 2017

@aminimalanimal aminimalanimal Update ssml.cleanse to stop removing spaces before decimals (i.e. “.2…
…oz, .5oz, .25oz” > “.2oz,.5oz,.25oz”)
cb9e394
@aminimalanimal aminimalanimal Update CHANGELOG
ebd37e2
Contributor

aminimalanimal commented May 5, 2017

Apologies—I'm uncertain why the Travis CI build is failing.

Collaborator

dblock commented May 5, 2017

The build failure is clearly about the changed behavior, so please run and fix the tests and maybe add more? LMK if you need help.

@aminimalanimal aminimalanimal Replace speak/break/etc tags with nothing instead of a space
690f80e
Contributor

aminimalanimal commented May 5, 2017

I just uploaded a change that passes the existing tests. Uncertain what the intention behind replacing speak/etc with a space was in the first place, so I don't know if this could potentially negatively impact something else.

Contributor

aminimalanimal commented May 5, 2017

btw, when I clicked on the travis-ci build bot earlier, it didn't provide me with any sort of failed test output like running npm run test does; it just had some generic failure messages about environment variables or something.

ericblade commented May 5, 2017 edited

umm.. a possible? point? might be that SSML is intended for the voice output, not the card output. Or is this some sort of auto-generated card, not a requested card?

In any case, I think that Alexa takes hints from the output punctuation, so not sure why we're pulling things out of it ..

Basically, I think I'm asking what is the point or purpose of this function?

Collaborator

dblock commented May 7, 2017

The way the change is implemented right now needs additional tests that would fail with the old code, please. Otherwise it's easy to reintroduce a regression.

When I code reviewed this first the output changes in tests made sense, now they are back to the original version, which is probably not what was intended?

Contributor

aminimalanimal commented May 8, 2017

@ericblade The cleanse method is run to “remove all SSML to keep the card clean,” so even though this is in a file named to-ssml, it's really only meant to be used when converting from SSML.

Contributor

aminimalanimal commented May 8, 2017

@dblock Looking into...

@aminimalanimal aminimalanimal Add test to prevent regression regarding truncation of spaces before …
…periods
7cbb9fa
Contributor

aminimalanimal commented May 8, 2017

When I code reviewed this first the output changes in tests made sense, now they are back to the original version, which is probably not what was intended?

It seems to me like the expectations behind all existing tests are still valid, so I'm not sure what you mean. I'm still seeing the expected output that I made the change for (which broke the tests), and removing the space that was added while stripping the SSML tags un-broke the tests.

I've added a test to prevent regression.

Thanks for your patience and explanations in this matter. I haven't done much TDD, so the thought to add a test to ensure that my change stays in place didn't occur to me.

Collaborator

dblock commented May 9, 2017

Looks good, thanks. Merging.

dblock merged commit f1d068d into alexa-js:master May 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 99.412%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment