search

alexa-js / alexa-app

A framework for Alexa (Amazon Echo) apps using Node.js
https://www.youtube.com/watch?v=pzM4jv7k7Rg
MIT License
1.03k stars 212 forks source link

Keep multiple newlines in card output (Continue PR #83) #373

Closed fremail closed 5 years ago

fremail commented 5 years ago

See PR #83 (2 years old hehe)

I merged the changes to current code and added tests.

kobim commented 5 years ago

Hi @fremail, thanks for your contribution!

Can you please sync merge from master so the Travis CI build will pass?

Also, I have the following doubts:

  1. If the only call to cleanse is in index.js, why there's a new parameter to the function? If this is the only usage, it should be the default behavior.
  2. I think the reason behind this regex is to remove whitespaces at the end and start of lines, and your code doesn't handle that. Maybe replacing the current patten with a custom version of \s will be better? (e.g. [ \t\r\f])
  3. Any reason you omitted tests from test_ssml.js? As the real change happens in a function related to to-ssml.js, I think the tests should occur there.

I think it's time we close this 2 years old issue :)

fremail commented 5 years ago

@kobim my answers

  1. I didn't change the original changes had been made in PR #83, but actually I agree with you: I don't see any reason in adding new param to the function.

  2. Oh, it's a good catch! Thank you for finding that bug in the change. I'll work to fix it.

  3. Really good question! I don't know why I did so :D

kobim commented 5 years ago

LGTM! I'll merge it to the master and will it will be released along with 4.2.3 (hopefully along with #378) this weekend.

Thanks again!