Added code to make slots optional#24
Conversation
|
You should write some tests and update README and CHANGELOG, please. |
|
@dblock Perhaps it would be a good ideal to add a literal checklist to CONTRIBUTING? Or a issue template? Also there's no mention of updating the changelog in CONTRIBUTING @dominicankev It looks like you used tabs instead of spaces for indenting your code. Please fix. |
|
@mreinstein is the boss here, but I support this message, maybe you can PR it? |
|
Hey guys. I apologize. Kind of new to the GitHub community and might have gotten a bit excited to post a commit. Should I cancel and update the files requested? |
|
@dominicankev No worries! PRs are (in general) good to see. As for updating this PR see here. |
|
@harrisonhjones Thanks! Will make the requested revisions. Thanks all! |
| @@ -111,15 +111,19 @@ test('exhaustive vs non-exhaustive expansion', function (t) { | |||
|
|
|||
| test('raw curly braces for custom slot types', function (t) { | |||
There was a problem hiding this comment.
In my opinion you should add a new test instead of repurposing an existing one.
test/index.js
Outdated
| "your favorite fruit is {Fruit} in {ROOM_NAME}", | ||
| "my least favorite fruit is {Fruit} in {ROOM_NAME}", | ||
| "your least favorite fruit is {Fruit} in {ROOM_NAME}", | ||
| "my favorite fruit is {Fruit} ", |
There was a problem hiding this comment.
Anyway to get rid of the hanging at the end of the line?
There was a problem hiding this comment.
@harrisonhjones If I trim the trailing space it seems to break the "optional terms" test which also has trailing spaces in the result. I can add a trim to the returned utterances ( ie: return utterances.trim(); ) but not sure if that would be desired or not.
There was a problem hiding this comment.
No doubt. Here's what I would do:
- Update the code to trim the whitespace (or not add it in the first place)
- Add a new test for the optional terms which doesn't include the trailing white space
- Run all the tests with your new code. They should all pass
- Report back if you have issues
There was a problem hiding this comment.
@harrisonhjones Cool. Thanks. Was thinking of doing that but wasn't sure if the trailing space was desired or not. Assumed it wasn't. It's more of a result of the utterance containing the space and the optional value not being there. Will give it a shot. Thanks again.
CHANGELOG.md
Outdated
| @@ -1,6 +1,6 @@ | |||
| ### Next | |||
| ### 0.2.2 | |||
There was a problem hiding this comment.
Don't add a version here, the maintainer will do that when they do a release.
There was a problem hiding this comment.
@dblock Thanks. Wasn't sure on that piece. Will update.
| ### Next | ||
| ### 0.2.2 | ||
|
|
||
| * Your contribution here. |
There was a problem hiding this comment.
Put this back for the next person please.
|
Fixed space vs tab issues and trimmed test to reflect change instead of using existing. |
|
Any idea if/when this PR will be merged? |
|
@dominicankev thanks for taking the time to submit. Please submit a clean PR that includes tests. |
5 participants
Added code to make slots optional by adding a "+" to the beginning and end of of desired slot name.
For example: {+OPTIONALSLOT+|}