Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Test phrases #146

Merged
merged 4 commits into from
Sep 12, 2017
Merged

Test phrases #146

merged 4 commits into from
Sep 12, 2017

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Sep 7, 2017

Added a convenience method for tokenizing a phrase, plus tests and test fixtures against that method.

Fixes #143.

/cc @mcwhittemore @lyzidiamond

@1ec5 1ec5 added the tests label Sep 7, 2017
@1ec5 1ec5 self-assigned this Sep 7, 2017
@1ec5 1ec5 mentioned this pull request Sep 7, 2017
@yuryleb
Copy link
Contributor

yuryleb commented Sep 8, 2017

It's not related to this PR directly but is it possible also to change arguments order of new tokenize(instruction, replaceTokens, language) function to move language argument first as in all other index.js's methods?

@1ec5
Copy link
Member Author

1ec5 commented Sep 8, 2017

@yuryleb, thanks for the suggestion. I opened #149 with that change.

Copy link
Contributor

@mcwhittemore mcwhittemore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence here.

Having used tokenize and compile to achieve this, I agree that a better way to access phrases would be nice. Accessing the JSON and then falling back to english is a bit much to then also need to send the language code to tokenize.

That said, compiling the steps as part of this process seems like we're going too far here. Part of me wonders how often this will be used vs just using compile and tokenize.

That said, I could see this being helpful if you were trying to reduce a set of steps into a non-one-to-one list of instructions so maybe its worth adding...

@1ec5
Copy link
Member Author

1ec5 commented Sep 11, 2017

I understand your hesitation around the additional compilePhrase() method. I ended up adding it mainly because it was necessary for unit testing phrase generation. I’d be fine with considering it a private method and not documenting it for clients (although it still needs to be exported for the tests).

@mcwhittemore
Copy link
Contributor

Can't the tests just call compile and then tokenize?

Omitted any English strings that came in for roundabout exits.
@1ec5
Copy link
Member Author

1ec5 commented Sep 12, 2017

Can't the tests just call compile and then tokenize?

Good point. Done.

@mcwhittemore
Copy link
Contributor

Cool. I think this ready to merge.

@1ec5 1ec5 merged commit b796d7e into master Sep 12, 2017
@1ec5 1ec5 deleted the 1ec5-phrase-test-143 branch September 12, 2017 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test phrases
3 participants