Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Throw error on nonalphanumeric namespace #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goodmike
Copy link

@goodmike goodmike commented Dec 30, 2016

[Closes #2]

Here's the PR to prevent users from accidentally choosing a namespace for a skill that will cause errors later.

I have 2 separate commits. The first sets up a minimal tape test environment and a single unit test for the error. If you're like me, you're more comfortable squashing a bug when there's a test to demonstrate it's fixed. However, if you don't want the dev dependency on tape, which also pulls a number of other packages in (even though it's supposed to be the lightweight testing alternative), or you prefer a different library like mocha or jasmine, feel free to ask me to discard or change this commit.

The second commit has the actual edit to chatskills.js. I decided to keep it as simple as I could, so I have a RegExp test for non-alphanumerics. I did not extract the namespace part of the RegExp expression in the session method because I think it would be difficult to use the partial pattern both in the add method and the session method. I don't think RegExps do not compose easily. Again, if you'd prefer I try to DRY things out, let me know.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using non-alphanumerics in app name causes opaque errors
1 participant